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 deprecated parameters from airflow (core) Operators #41736

Conversation

jscheffl
Copy link
Contributor

Cleanup as deprecation promised for Airflow 3

Note: Python Operators are already in PR #41493

@jscheffl jscheffl added airflow3.0:candidate Potential candidates for Airflow 3.0 airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes labels Aug 25, 2024
@jscheffl jscheffl added this to the Airflow 3.0.0 milestone Aug 25, 2024
@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Aug 25, 2024
@potiuk potiuk merged commit 27fe45b into apache:main Aug 27, 2024
53 checks passed
@eladkal
Copy link
Contributor

eladkal commented Aug 27, 2024

We need to revert this we can't merge this before we release the standard provider
#41564

@potiuk
Copy link
Member

potiuk commented Aug 27, 2024

We need to revert this we can't merge this before we release the standard provider

Why ? I thought standard provider will not have deprecated stuff?

@eladkal
Copy link
Contributor

eladkal commented Aug 27, 2024

Why ? I thought standard provider will not have deprecated stuff?

We must have version 1.0.0 or the provider identical to the operator code of 2.10.0
We can't have the initial version having hidden changes.

After we release provider 1.0.0 we can remove all deprecations and cut 2.0.0
The only change to main branch should be removing the modules, it shouldn't be about what is changed in the modules this is done from the provider side.

@potiuk
Copy link
Member

potiuk commented Aug 27, 2024

We must have version 1.0.0 or the provider identical to the operator code of 2.10.0
We can't have the initial version having hidden changes.

I am not sure if I agree. We have not discussed it, and that's why I asked @ashb how he imagines the new "standard" provider migration to look like - but Airflow 2.10/2.11 will have "old" operators (and people might continue using them - there is no particular need to have "standard" provider to be 100% compatible - especially that vast majority of those deprecations is there for a long time (this is in a stark contrast with "templates" where we never told our users that they shoudl rewrite their templates and they will not have a way to do it for some time..

Having a "standard" provider installed "additionaly" with all the deprecations removed, will be a great opportunity to not only remove deprecations but make sure the that deprecated parameters are not used. And users will be able to do in Airlfow 2.10 / 2.11 dag-by-dag (because they will have both version of standard operators - one in "airflow.core" and one in "airflow.providers.standard" - then they will be able to change the imports and make sure all works before they migrate to Airflow 3.

Or at least that is my thiniking about it.

@eladkal
Copy link
Contributor

eladkal commented Aug 27, 2024

@potiuk regardless of the greater issue. We cant release the provider without revert this PR.

This PR removed code from datetime operators. The removal notice will appear in Airflow 3 changelog. The current provider PR has min airflow version of 2.8
Users who will start using it will experience breaking changes without notice.

In the past everytime we created a new provider we kept consistency between the code from the migrated part to the new provider allowing users to just replace import paths and everything will work out of the box. I am looking to give users the exact same experience.

@potiuk
Copy link
Member

potiuk commented Aug 27, 2024

This PR removed code from datetime operators. The removal notice will appear in Airflow 3 changelog. The current provider PR has min airflow version of 2.8

Again - we have not discussed it, but I see the 'standard" provider as 3.0 feature coming mostly from the AIP-72 needs (@ashb ?). Or at least this is where the idea was raised. We might allow them to be used for Airflow 2 but mostly as a bridge to airflow 3 and we are not (unlike for other providers) going to remove them from airflow core (or at least I am not awre of that) - so the airflow.operators.python.Python might be (and will be) different than airflow.providers.standard.python.PythonVirtualenvOperator in Airflow 2 and they should happily co-exist - which is IMHO a great opportunity to gradually move stuff for user's DAGs.

In the past everytime we created a new provider we kept consistency between the code from the migrated part to the new provider allowing users to just replace import paths and everything will work out of the box. I am looking to give users the exact same experience.

I am not sure if this is our goal this time. It might be we decide that, but I am also not sure if we wanted to do it this way. I Thinl we could likely go either way, but I am not sure we agreed to that.

@potiuk
Copy link
Member

potiuk commented Aug 27, 2024

Simply speaking - I see "standard" as very similar thing to backport providers we had in Airflow 1 -> Airflow 2... But again, maybe my vision /approach is wrong.

@potiuk
Copy link
Member

potiuk commented Aug 27, 2024

(also for back-compatibiliity - at least until we move quite some other stuff to the provider, it can't have >= 2.8 because it currently uses some classes thet have been added in Airflow 2.10 -> see #41564 (comment)

I think we should really decide what is the purpose of the standard provider..

@eladkal
Copy link
Contributor

eladkal commented Aug 27, 2024

OK. Then we need to discuss...
Even if the provider is 3.0+ only I still think this PR needs to be reverted. Version 1.0.0 of the provider should be identical to the core code. Then 2.0 can remove anything we want.

The core change log should not contain anying other than - module is removed.
All the details of parmaters/function changes should be in provider change log.

cc @kaxil

@jscheffl
Copy link
Contributor Author

OK. Then we need to discuss... Even if the provider is 3.0+ only I still think this PR needs to be reverted. Version 1.0.0 of the provider should be identical to the core code. Then 2.0 can remove anything we want.

The core change log should not contain anying other than - module is removed. All the details of parmaters/function changes should be in provider change log.

cc @kaxil

Also answered in Slack. I think my opinion is with Jarek. If we spin a new provider we should not carry deprecated legacy with it. Somebody migrating needs yto take care for deprecations.

Also I see the new provider earliest in the bridge release as additional package. No need to be make it compatible with 2.8... also nothing speaking against making it compatible with 2.8 but still should not carry deprecated functions.

So good that we are not missing agenda items for the Dev calls... :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes airflow3.0:candidate Potential candidates for Airflow 3.0 area:core-operators Operators, Sensors and hooks within Core Airflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants