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

Deprecation of AutoML services: Add deprecation warnings and raise exceptions for already deprecated ones #38673

Merged
merged 1 commit into from
May 10, 2024

Conversation

molcay
Copy link
Contributor

@molcay molcay commented Apr 2, 2024

Before merging this PR we need to merge #38633

This PR is for raising exception for the deprecated services of AutoML (Tables, Vision, Video Intelligence, Natural Language) which are EoL or will be EoL soon.


^ 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:providers provider:google Google (including GCP) related issues labels Apr 2, 2024
@molcay molcay changed the title Add deprecation warnings and raise exception for already deprecated ones Deprecation of AutoML services: Add deprecation warnings and raise exceptions for already deprecated ones Apr 2, 2024
@potiuk
Copy link
Member

potiuk commented Apr 5, 2024

I think you need to rebase the PR. It's 52 commits behind and this is likely why it fails

@molcay molcay force-pushed the deprecate-automl branch 3 times, most recently from 8d86abd to 338ca37 Compare April 9, 2024 08:34
Copy link
Collaborator

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

Good start!
There's some work to do to finish it up (mostly adding tests and docstrings), and I hope that my comments will make sense - please ask if you have any questions.
I'd be happy if you could test the operator against an actual GCP project with enabled AutoML API to see if all functionalities work as expected :)

airflow/providers/google/cloud/operators/automl.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/operators/automl.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/operators/automl.py Outdated Show resolved Hide resolved
tests/providers/google/cloud/operators/test_automl.py Outdated Show resolved Hide resolved
tests/providers/google/cloud/operators/test_automl.py Outdated Show resolved Hide resolved
airflow/providers/google/cloud/operators/automl.py Outdated Show resolved Hide resolved
@molcay molcay force-pushed the deprecate-automl branch 3 times, most recently from 9f17725 to 1554e19 Compare April 15, 2024 09:18
@potiuk
Copy link
Member

potiuk commented Apr 23, 2024

@molcay -> can you please rebase/resolve conflict and respond/mark as resolved all conversations that you think were resolved?

@molcay molcay force-pushed the deprecate-automl branch 2 times, most recently from c646a71 to ec88665 Compare April 24, 2024 14:54
@potiuk
Copy link
Member

potiuk commented Apr 24, 2024

cc: @VladaZakharova - there were some doubts for Google team about those deprecations - does this one look ok for you?

@VladaZakharova
Copy link
Contributor

cc: @VladaZakharova - there were some doubts for Google team about those deprecations - does this one look ok for you?

Hi! Thanks, yes, changes look good, LGTM

@potiuk
Copy link
Member

potiuk commented Apr 25, 2024

@molcay -> can you please review and address/mark as resolved if they are - the comments made by @shahar1 ?

@molcay
Copy link
Contributor Author

molcay commented Apr 26, 2024

@potiuk,
Ohh I see, there are some hidden conversations. I totally missed them. Sorry for the inconvenience. I will try to address and resolve them as soon as possible.

Copy link
Collaborator

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

Very good progress! :)
You resolved all of my comments, so you get a sincere approval from me.
One last step before merging - try to rebase and ensure that tests/providers/google/cloud/operators/test_automl.py passes in the Non-DB tests, as currently they fail on the CI.
Make sure that objects are mocked properly in tests that you added or modified.

@molcay
Copy link
Contributor Author

molcay commented May 2, 2024

I will work on this test failure and send an update. Thank you for the review @shahar1
Also, thank you for pinging and guiding along the way @potiuk

@molcay molcay force-pushed the deprecate-automl branch 2 times, most recently from 57b7fc4 to fec27f3 Compare May 2, 2024 09:07
@VladaZakharova
Copy link
Contributor

Hey @shahar1 !
Would be nice if you can check the changes here again

@eladkal eladkal requested a review from shahar1 May 10, 2024 05:42
Copy link
Collaborator

@shahar1 shahar1 left a comment

Choose a reason for hiding this comment

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

Re-reviewed, we're good to go :)

@potiuk potiuk merged commit 8dcee5b into apache:main May 10, 2024
67 checks passed
RodrigoGanancia pushed a commit to RodrigoGanancia/airflow that referenced this pull request May 10, 2024
pateash pushed a commit to pateash/airflow that referenced this pull request May 13, 2024
@utkarsharma2 utkarsharma2 added type:improvement Changelog: Improvements changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed type:improvement Changelog: Improvements labels Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants