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

Redirect old location module imports to standard provider #43610

Conversation

gopidesupavan
Copy link
Member

@gopidesupavan gopidesupavan commented Nov 2, 2024

We have moved the operators/sensors/hooks/utils to standard provider from core area location.

This change is to support in ariflow 3.0.0 when user tries to import from old location,
it redirects to standard provider.

So far the following operators/sensors/hooks/utils has been moved to standard provider.

#41564
#42506
#42081
#42252

related: #43582
related: #43641


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@gopidesupavan
Copy link
Member Author

@ashb If you feel this is not correct :), please suggest any alternatives or if there is more optimised way to workout these.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

nice. @ashb ?

@gopidesupavan
Copy link
Member Author

Added tests to check if imports are working or not :)

@gopidesupavan gopidesupavan force-pushed the redirect-core-module-references-to-standard-provider branch from 6ff618f to 57337f6 Compare November 4, 2024 15:34
@gopidesupavan gopidesupavan force-pushed the redirect-core-module-references-to-standard-provider branch from 8f923be to c6eaf74 Compare November 5, 2024 04:56
@romsharon98 romsharon98 force-pushed the redirect-core-module-references-to-standard-provider branch from c6eaf74 to 969da8d Compare November 5, 2024 09:50
@potiuk potiuk merged commit 5de2e73 into apache:main Nov 12, 2024
57 checks passed
@potiuk
Copy link
Member

potiuk commented Nov 12, 2024

Should we somehow check if "all" moved imports are handled? Not sure if we can do it easily though.

sunank200 pushed a commit to astronomer/airflow that referenced this pull request Nov 12, 2024

---------

Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
potiuk added a commit to potiuk/airflow that referenced this pull request Nov 13, 2024
…ache#43610)"

There is a very interesting breaking change introduced in Python 3.11
that will likely mean that we should not use MetaPathFinder for old
standard provider classes redirection.

There was a change introduced in Python 3.11 that caused that
module might not be found in some cases (for example when
unit test patches the path) when the module is loaded as
a different module (i.e. a.b differs from sys.modules['a.b'])

This is tracked in CPython via:
python/cpython#117860

This causes standard operator's tests fail in Python 3.11 and
3.12 when the providers/tests/standard/test_module_redirect_finder.py
is executed before - i.e. the standard modules are loaded as
old modules.

This reverts commit 5de2e73.
gopidesupavan pushed a commit that referenced this pull request Nov 13, 2024
…3610)" (#43946)

There is a very interesting breaking change introduced in Python 3.11
that will likely mean that we should not use MetaPathFinder for old
standard provider classes redirection.

There was a change introduced in Python 3.11 that caused that
module might not be found in some cases (for example when
unit test patches the path) when the module is loaded as
a different module (i.e. a.b differs from sys.modules['a.b'])

This is tracked in CPython via:
python/cpython#117860

This causes standard operator's tests fail in Python 3.11 and
3.12 when the providers/tests/standard/test_module_redirect_finder.py
is executed before - i.e. the standard modules are loaded as
old modules.

This reverts commit 5de2e73.
@gopidesupavan
Copy link
Member Author

Should we somehow check if "all" moved imports are handled? Not sure if we can do it easily though.

Yeah have ran basic python operator but that is on 3.9 versions. now that MethPathFinder not supported in 3.11 and 3.12, so will check if we can do anything :)

@potiuk
Copy link
Member

potiuk commented Nov 13, 2024

Yeah have ran basic python operator but that is on 3.9 versions. now that MethPathFinder not supported in 3.11 and 3.12, so will check if we can do anything :)

Just to document: MetaPath solution suffers from bug introduced in Python 3.11 python/cpython#117860 which is affecting unittest mock imports but also apparently goes beyond just unit tests - for example affecting Apprise 1.8 PrefectHQ/prefect#13314 after they implemented refactors internally.

Maybe we can come back (@ashb ?) to the old ways of doing it - we had implemented deprecation utils in v1-> v2 migration that we can apply again: https://github.com/apache/airflow/blob/v2-10-test/airflow/operators/__init__.py - they did the job, the only problem was that the old "package" will have remain there - with an __init__.py with all the deprecation dicts. Which is "slightly" annoying, but also has the benefit of it being pretty explicit, as opposed to MetaPath thing. You know where you should look for it at least. And it will not suffer from the 3.11+ bug of importing packages that are loaded from a different path - because those are "real" packages and real modules not "linked" modules.

Also - I think such __init__.py file can be easily generated from checked out v2-10-test branch - we could have a simple script that will import all the "airflow.operators", "airflow.hooks" etc and produce it in fully automated way, this way we will not have to verify it (and since we will never be changing it, we can generate it now and re-generate just before we release Airflow 3, to acount for potential Airflow 2.11 changes. This was following PEP 562 __getattr__ https://peps.python.org/pep-0562/ and has been working 100% reliably so far.

WDYT @gopidesupavan @ashb -> I think while the MetaPath was a nice idea, but it's a bit too risky.

@potiuk
Copy link
Member

potiuk commented Nov 13, 2024

After discussion in Slack @gopidesupavan -> maybe you could (at least for now - attempt to follow the PEP-562 approach and redo the change and we can revisit it later). Ideally simple script to generate moved class dicts would be cool to have so that we can re-apply it as we move new classes.

@gopidesupavan
Copy link
Member Author

After discussion in Slack @gopidesupavan -> maybe you could (at least for now - attempt to follow the PEP-562 approach and redo the change and we can revisit it later). Ideally simple script to generate moved class dicts would be cool to have so that we can re-apply it as we move new classes.

Yeah agree, better to avoid metapath. let me get the changes with PEP 562 __getattr__

@potiuk
Copy link
Member

potiuk commented Nov 13, 2024

BTW. This is a bit of an assumption that PEP 562 is going to be "ok" - it should work as this really creates aliases to objects in a different package, and we did not have problems with Airflow 2 deprecations even on airflow 2.11 and 2.12 - so it's likely to work (but let's see :).)

@potiuk
Copy link
Member

potiuk commented Nov 13, 2024

And BTW. when you create a PR - you can assign "all versions" and "full tests needed" labels to your PR @gopidesupavan before you create it - this way it will run on all versions of Python/Databases and K8S (and will trigger all tests)

ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024

---------

Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
…ache#43610)" (apache#43946)

There is a very interesting breaking change introduced in Python 3.11
that will likely mean that we should not use MetaPathFinder for old
standard provider classes redirection.

There was a change introduced in Python 3.11 that caused that
module might not be found in some cases (for example when
unit test patches the path) when the module is loaded as
a different module (i.e. a.b differs from sys.modules['a.b'])

This is tracked in CPython via:
python/cpython#117860

This causes standard operator's tests fail in Python 3.11 and
3.12 when the providers/tests/standard/test_module_redirect_finder.py
is executed before - i.e. the standard modules are loaded as
old modules.

This reverts commit 5de2e73.
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.

6 participants