Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compat module for typing airflow.utils.context.Context #619

Merged
merged 7 commits into from
Sep 14, 2022

Conversation

josh-fell
Copy link
Contributor

@josh-fell josh-fell commented Sep 3, 2022

The airflow.utils.context module was not introduced to OSS Airflow until 2.2.3. Importing this module as a top-level import sets an implicit, minimum requirement for Airflow 2.2.3 which is higher than the minumum apache-airflow requirement set for Astronomer Providers. This PR adds a typing compat module to be used throughout the repo for consistent typing of context.

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #619 (12f3334) into main (a07a98e) will increase coverage by 0.00%.
The diff coverage is 97.72%.

@@           Coverage Diff           @@
##             main     #619   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          78       79    +1     
  Lines        4127     4133    +6     
=======================================
+ Hits         4057     4063    +6     
  Misses         70       70           
Impacted Files Coverage Δ
astronomer/providers/utils/typing_compat.py 66.66% <66.66%> (ø)
astronomer/providers/amazon/aws/operators/batch.py 100.00% <100.00%> (ø)
astronomer/providers/amazon/aws/operators/emr.py 100.00% <100.00%> (ø)
...providers/amazon/aws/operators/redshift_cluster.py 98.63% <100.00%> (+1.33%) ⬆️
...er/providers/amazon/aws/operators/redshift_data.py 100.00% <100.00%> (ø)
...mer/providers/amazon/aws/operators/redshift_sql.py 96.42% <100.00%> (ø)
astronomer/providers/amazon/aws/sensors/batch.py 100.00% <100.00%> (ø)
astronomer/providers/amazon/aws/sensors/emr.py 100.00% <100.00%> (ø)
...r/providers/amazon/aws/sensors/redshift_cluster.py 100.00% <100.00%> (ø)
astronomer/providers/amazon/aws/sensors/s3.py 100.00% <100.00%> (ø)
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@josh-fell josh-fell marked this pull request as draft September 7, 2022 03:36
@josh-fell josh-fell force-pushed the context branch 2 times, most recently from ca8f84d to b08811a Compare September 7, 2022 18:14
@josh-fell josh-fell changed the title Move airflow.utils.context.Context import under TYPE_CHECKING Add compat module for typing airflow.utils.context.Context Sep 7, 2022
from airflow.utils.context import Context
except ModuleNotFoundError:

class Context(MutableMapping[str, Any]): # type: ignore[no-redef]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the approach of Context = MutableMapping[str, Any], mypy was complaining about not being able to assign to a type and the variable assignment types not matching. Instead of ignoring those mypy errors (which felt icky), I decided to redefine the typing class and ignore the redefinition mypy error.

Does this seem reasonable? Any other mypy workarounds?

@josh-fell
Copy link
Contributor Author

josh-fell commented Sep 8, 2022

The pre-commit.ci check is failing for Executable '.circleci/scripts/pre_commit_context_typing_compat.py' is not executable. I'm not sure how to resolve this. Both make run-static-checks and pre-commit run --all-files execute just fine locally for me. Any pointers?

@josh-fell josh-fell marked this pull request as ready for review September 8, 2022 02:14
@josh-fell
Copy link
Contributor Author

Of course, if there is a preference of having unit test over a pre-commit hook, I can change that no problem!

@bharanidharan14
Copy link
Contributor

@josh-fell Still code coverage is failing

The `airflow.utils.context` module was not introduced to OSS Airflow until 2.2.3. Importing this module as a top-level import sets an implicit, minimum requirement for Airflow 2.2.3 which is higher than the minumum `apache-airflow` requirement set for Astronomer Providers. This PR adds a typing compat module to be used throughout the repo for consistent typing of `context`.
@josh-fell
Copy link
Contributor Author

@josh-fell Still code coverage is failing

Ah yes. Thanks for the poke. Just pushed a unit test in e2a9ade which hopefully helps.

@josh-fell
Copy link
Contributor Author

Good to know the pre-commit hook works. Updated the dbt Cloud modules.

@kaxil kaxil merged commit 14751af into astronomer:main Sep 14, 2022
@josh-fell josh-fell deleted the context branch September 14, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants