Removed deprecated contrib files and replace them with PEP-562 getattr #26153
Conversation
|
I attempted to replace all the contrib classes with PEP-562 ones. Looking forward to comments. Also the presence of the tool might be good as part of review - to see that I have not missed anything. This change is -10K lines, so likely easier to review the "method", than the result. All the __deprecated_classes were generated using this tool. Few things I would like to get comments on (It's the first time I apply PEP-562):
And if you think of it - it makes sense. We generated the modules out of thin air, so they do not have spec to use to reload them. And those tests are I think quite a bit redundant now - since we know the warnings are generated by |
|
BTW. This loooks really cool:
|
potiuk
force-pushed
the
remove-deprecated-contrib-classes
branch
3 times, most recently
from
3f3cca2
to
669173c
Compare
airflow/utils/deprecation_tools.py
Outdated
|
|
||
| def add_deprecated_classes(module_imports: Dict[str, Dict[str, str]], module: str): | ||
| for submodule_name, imports in module_imports.items(): | ||
| module_type = ModuleType(f"{module}.{submodule_name}") |
There was a problem hiding this comment.
Not sure that it matters, but this approach does "pollute" sys.modules with all deprecated modules when any contrib module is loaded.
There was a problem hiding this comment.
Yeah. It does. That was my worry too.
But I think this does not really matter - those are super small modules (just getattr methods and the dictionaries) and i would say price to pay to access contrib for the users :). We WANT the friction to be there :).
Do you think it's worth to do it differently ?
There was a problem hiding this comment.
Not sure if it works, but can you put a __getattr__ on airflow.contrib.hooks that "loads" each module on demand instead?
There was a problem hiding this comment.
Tried it, and it does not work, module loader does not use __getattr__, it uses much more complex machinery to find modules. By default (with defalt modulespec and loader) it expects the Package object (which should be added to sys.modules[package])' to return pathattribiute (list of paths to search). I even went as far as creating this dynamically created package but then I have nopaths` to provide it to :).
I think we would have to hack and change the loader implementation to not search in __path__ but implement ModuleSpec based on https://peps.python.org/pep-0451 I believe. This would also eventually enable complete removal of sub-packages in contrib, but I somehow feel it's not worth it as it's a bit too magical IMHO and we do not need the whole machinery to reload the modules which is behind it).
The nice thing about this solution is that it's fairly straightforward to get how it works and it's pretty much based on simple standards (PEP 562 and the fact that all modules should land in sys.modules).
There was a problem hiding this comment.
BTW. I updated the naming in the proposed solution - replacing module with package and submodule with module 🤦
There was a problem hiding this comment.
Oh yeah, totally not worth building a custom loader mechanism for this!
The conrib modules are carried from Airflow 1.10 and we cannot remove the classes that were used there without bumping Airlfow to 3, however we can replace the files with dynamic attribute retrieval following PEP-562. This way we will have far less number of files in contrib and users will not get auto-import and autocomplete when they will be developing their DAGs. This does not break compatibility but it providers much stronger signal to the users that they should switch to new classes: * they will not see the classes in autocomplete and autoimport any more * it will seem that the classes are not existing in contrib until they are used * if anyone performs static analysis on the DAGs, the analysis will fail (and this is an intended behaviour)
potiuk
force-pushed
the
remove-deprecated-contrib-classes
branch
from
669173c
to
bcfeb47
Compare
|
This is what we get now:
|
The apache#26153 missed the classes that were added by deriving a new class from the imported ones. This was mostly about non-consistent spelling of class names. This PR fixes it and brings missing contrib classes back.
The #26153 missed the classes that were added by deriving a new class from the imported ones. This was mostly about non-consistent spelling of class names. This PR fixes it and brings missing contrib classes back.
ephraimbuddy
added
the
type:misc/internal
The conrib modules are carried from Airflow 1.10 and we cannot
remove the classes that were used there without bumping Airlfow
to 3, however we can replace the files with dynamic attribute
retrieval following PEP-562. This way we will have far less number
of files in contrib and users will not get auto-import and
autocomplete when they will be developing their DAGs.
This does not break compatibility but it providers much stronger
signal to the users that they should switch to new classes:
any more
they are used
will fail (and this is an intended behaviour)
^ 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.rstor{issue_number}.significant.rst, in newsfragments.