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

Remove remaining deprecated classes and replace them with PEP562 #26167

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Sep 5, 2022

This is a follow-up after #26153 - removal of all remaining
deprecated classes and replace them with PEP-562 dynamic attribute
loading.


^ 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.

@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Sep 5, 2022
@potiuk
Copy link
Member Author

potiuk commented Sep 5, 2022

This feels SO. MUCH. BETTER.

Compare:

image

with:

image

image

image

@potiuk potiuk force-pushed the remove-remaining-deprecated-classes branch from 69e5c36 to 32b67d2 Compare September 5, 2022 21:48
@potiuk
Copy link
Member Author

potiuk commented Sep 5, 2022

I had to manually adjust the changes a bit - there was one case I have not automatically pick up (when class was extending from deprecated class) - I will take a close look at the contrib ones if there was no similar case.

@potiuk potiuk force-pushed the remove-remaining-deprecated-classes branch from 32b67d2 to d483675 Compare September 5, 2022 22:17
@potiuk
Copy link
Member Author

potiuk commented Sep 5, 2022

@eladkal @ashb @uranusjr @mik-laj -> there was a bt of trouble with the "DummyOperator" - the tests started to fail (and for a good reason). There was a custom logic for the inheritance check and "is_empty" but as I understand the reason behind the logic of the check, after this PEP check we do not need the custom logic any more.

The difference with PEP 562, we are not extending the EmptyOperator any more (to create a DummyOperator that can be imported via from airflow.operators.dummy import DummyOperator. With PEP 562, the operator imported via from airflow.operators.dummy import DummyOperator IS EmptyOperator, so the check for serialization and "is_empty" will just work. But I wold love if someone verifies my logic :).

@potiuk
Copy link
Member Author

potiuk commented Sep 5, 2022

As a bonus point - the "dummy" import will raise DeprecationWarning immediately rather than when the DummyOperator is instantiated (so test_dummy.py can also be safely removed - it was testing if the warning was generated at instantiation time).

@potiuk
Copy link
Member Author

potiuk commented Sep 5, 2022

I will also have to update the contrib with the case where we had "extending" classes. Stay tuned :)

@potiuk potiuk force-pushed the remove-remaining-deprecated-classes branch from d483675 to e5829fa Compare September 6, 2022 14:27
@potiuk
Copy link
Member Author

potiuk commented Sep 6, 2022

Ok. Fixed missing classes automatically.

@potiuk potiuk force-pushed the remove-remaining-deprecated-classes branch from e5829fa to 7b92ca2 Compare September 6, 2022 14:31
@potiuk
Copy link
Member Author

potiuk commented Sep 6, 2022

missing contrib classes added in #26179

@potiuk potiuk added this to the Airflow 2.4.0 milestone Sep 6, 2022
@potiuk
Copy link
Member Author

potiuk commented Sep 7, 2022

:D ?

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

I've done a quick-n-dirty checkof performance costs -- because importing airflow.hooks.subprocess (which isn't deprecated) and it seems to have no difference to performance outside of the variance (0.26-0.29s to import airflow.hooks.process in a new python process on my laptop.)

One downside (which I don't think has any real tangible cost) is that importing airflow.operators.bash now has 66 extra modules as a result of the code in airflow/operators/__init__.py. I think that's fine.

@ashb
Copy link
Member

ashb commented Sep 7, 2022

@eladkal @ashb @uranusjr @mik-laj -> there was a bt of trouble with the "DummyOperator" - the tests started to fail (and for a good reason). There was a custom logic for the inheritance check and "is_empty" but as I understand the reason behind the logic of the check, after this PEP check we do not need the custom logic any more.

The difference with PEP 562, we are not extending the EmptyOperator any more (to create a DummyOperator that can be imported via from airflow.operators.dummy import DummyOperator. With PEP 562, the operator imported via from airflow.operators.dummy import DummyOperator IS EmptyOperator, so the check for serialization and "is_empty" will just work. But I wold love if someone verifies my logic :).

Yes, your logic is sound

@ashb
Copy link
Member

ashb commented Sep 7, 2022

@potiuk Conflicting now -- can you resolve so we can merge this and get 2.4 beta out today?

This is a follow-up after apache#26513 - removal of all remaining
deprecated classes and replace them with PEP-562 dynamic attribute
loading.
@potiuk potiuk force-pushed the remove-remaining-deprecated-classes branch from 7b92ca2 to cf69464 Compare September 7, 2022 20:18
@potiuk
Copy link
Member Author

potiuk commented Sep 7, 2022

@potiuk Conflicting now -- can you resolve so we can merge this and get 2.4 beta out today?

rebased. Merging shortly.

@potiuk potiuk merged commit 63562d7 into apache:main Sep 7, 2022
@potiuk potiuk deleted the remove-remaining-deprecated-classes branch September 7, 2022 20:20
@potiuk
Copy link
Member Author

potiuk commented Sep 7, 2022

Merged preemptively - there is nothing there that would break after merge.

@potiuk
Copy link
Member Author

potiuk commented Sep 7, 2022

One downside (which I don't think has any real tangible cost) is that importing airflow.operators.bash now has 66 extra modules as a result of the code in airflow/operators/__init__.py. I think that's fine.

Yeah. Those imports are really equivalent to parsing literally very few init,.py files, parsing the dict and creating 60 very simple objects in memory with one getattr function added to them. None of the actual referred classes are imported until they are used, so all the heavy-lifting is deferred. This is far less involved than many other "import" overhead, where importing one package might drag walking through a number of folders and files, and parsing multiple of them and instantiating multiple complex objects

It is literally as "lightweight" as it can be. I even thoguht about doing some kind of lazy-proxy style of module initialization, but I think those instantiated modules are even more light-weight than any lazy-proxy style objects - they are pretty much lazy-proxies themselves.

@potiuk
Copy link
Member Author

potiuk commented Sep 7, 2022

I think what really helps here is that we have literally one __init__.py file to process per package and that everything (one dict) is inside of that single file - even the overhead on opening /closing the single file is negligible in this case.

@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants