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

Revert "Delete deprecated AutoML operators and deprecate AutoML hook and links (#38418)" #38633

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

shahar1
Copy link
Contributor

@shahar1 shahar1 commented Mar 30, 2024

This reverts commit 4a4eee1 (#38418).

As @molcay stated in #38418, the operators and hook may still be useful for AutoML Translation, which I overlooked when working on the PR above.
I'll create another PR to delete specific ops. and hooks that become unusable and to convert some deprecation warnings to exceptions.
I'll try to be more careful on similar occasions.


^ 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 area:dev-tools area:production-image Production image improvements and fixes area:providers area:system-tests kind:documentation provider:google Google (including GCP) related issues labels Mar 30, 2024
@eladkal
Copy link
Contributor

eladkal commented Mar 30, 2024

I assume this is due to #38418 (comment) ?

Can you please clarify what APIs are not going to be useful after Mar 31? Because if the APIs are not useful we don't need to revert

@shahar1
Copy link
Contributor Author

shahar1 commented Mar 30, 2024

I assume this is due to #38418 (comment) ?

Can you please clarify what APIs are not going to be useful after Mar 31? Because if the APIs are not useful we don't need to revert

I assume this is due to #38418 (comment) ?

Can you please clarify what APIs are not going to be useful after Mar 31? Because if the APIs are not useful we don't need to revert

Yes, the following APIs won't be available after Mar 31: AutoML Tables, AutoML Video Intelligence, AutoML Vision and AutoML NLP.
However, there is one API called AutoML Translation which could be utilized by some of the operators/hooks (not all of them).
I'll create a separate PR to delete operators and hooks that couldn't be utilized by this API.

@eladkal
Copy link
Contributor

eladkal commented Mar 30, 2024

tests are failing due to merging #38504
you will need to add commit to add the needed files to the ignore yaml

@eladkal
Copy link
Contributor

eladkal commented Mar 31, 2024

cc @potiuk I am not sure how we can workaround the error build?

@molcay
Copy link
Contributor

molcay commented Apr 2, 2024

Hello @shahar1, Thank you for this PR and the previous PR. I tried to use the revert button for the previous PR but it did not work.
Actually, I was about to create a PR to make necessary changes and am working on another fork (here is the PR), but when I created the PR it said that there are conflicts. So I put the comment to the previous PR.

When I create the community PR (to this repo), I will ask a review from you @shahar1

@shahar1
Copy link
Contributor Author

shahar1 commented Apr 2, 2024

Hello @shahar1, Thank you for this PR and the previous PR. I tried to use the revert button for the previous PR but it did not work.
Actually, I was about to create a PR to make necessary changes and am working on another fork (here is the PR), but when I created the PR it said that there are conflicts. So I put the comment to the previous PR.

When I create the community PR (to this repo), I will ask a review from you @shahar1

Hey, there was a conflict due to changes of dependcies handling in pyproject.toml.
I've already created PR #38635 for the deprecation here - I didn't know that you've already created this PR, as you created it in Vlada's fork and not here :)
After merging the current PR - you may recreate the PR here, I'll close mine and review yours - or we do the other way around, let me know whay works best for you.

@molcay
Copy link
Contributor

molcay commented Apr 2, 2024

Hey @shahar1,

Thank you again for your effort and struggles for this matter; very appreciated.


...I didn't know that you've already created this PR, as you created it in Vlada's fork and not here :)

Yes, I know I created the PR in Vlada's fork 😅 It was my fault for not mentioning the WIP PR. Sorry for the miscommunication.
Vlada's fork is our first place to create PR and get internal review then we are creating the PR to this repo. When I was creating the PR in the Vlada's fork, I saw that there were (and still are, since this is not merged yet) some conflicts I put that comment to #38418. But couldn't revert (not before you at least 😅)

Btw, I created the new PR: #38673


After merging the current PR - you may recreate the PR here, I'll close mine and review yours ...

If we can do this, it will be wonderful because for the PR I tried to comply with the Airflow Deprecation Policy.

e.g: Giving alternative operators instead of the deprecated and/or shutdown services' operators

Also, we will continue to support most of the operators (at least for the next major version of the package) due to the deprecation policy. They will definitely fail and we will raise an exception for the users who are using the operators but it needs to be there. Hence we cannot remove the system tests.

Also, the main idea of the new PR is finding alternatives/replacements of the operators for the deprecated services instead of just removing them.


To summarize the current situation,
We have 4 PRs:

PR Summary
#38418 Merged and need to be reverted
#38633 This PR reverting #38418
#38635 The other PR related to this topic
#38673 This is the new PR that contains policy complaint changes
#38636 The issue related to this topic

@potiuk potiuk added default versions only When assigned to PR - only default python version is used for CI tests include success outputs Whether to include success outputs in test results labels Apr 2, 2024
@potiuk potiuk merged commit c443971 into apache:main Apr 2, 2024
67 checks passed
idantepper pushed a commit to idantepper/airflow that referenced this pull request Apr 3, 2024
idantepper pushed a commit to idantepper/airflow that referenced this pull request Apr 3, 2024
mathiaHT pushed a commit to mathiaHT/airflow that referenced this pull request Apr 4, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
@shahar1 shahar1 deleted the revert-38418 branch June 12, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:production-image Production image improvements and fixes area:providers area:system-tests default versions only When assigned to PR - only default python version is used for CI tests include success outputs Whether to include success outputs in test results kind:documentation provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants