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 "Deprecation of AutoML services: Add deprecation warnings and raise exceptions for already deprecated ones" #39753

Closed
wants to merge 1 commit into from

Conversation

Taragolis
Copy link
Contributor

Reverts #38673

This PR constains explicit change filter warnings in AutoMLTablesUpdateDatasetOperator and AutoMLDeployModelOperator by provide action="error" into the decorator which looks like an worst practice, because it can't be controlled by the end users.

If this functional still exists and users could use it we should not raise an error here, user might want to control it by provide -W flag to the interpreter or PYTHONWARNINGS env var: see Warning control

If this functional not exists we could:

  • Just raise a warning, so we would warn end users that this functional not exists
  • Explicitly raise AirflowException (we do not have appropriate exception, even if we add it it will available in Airflow 2.10+) in constructor (__init__) method

There is difference deprecated and removed`. Deprecated mean this functional still available (even if limited) but removed mean that this functional not available anymore. We should not use deprecated meaning in case when functional removed.

There is an option to change by the separate PR instead of this revert.
cc: @VladaZakharova @molcay @shahar1

@molcay
Copy link
Contributor

molcay commented May 22, 2024

Hi @Taragolis,
I think reverting to PR is not a good idea. We changed a lot of operators and the services were all ready in the sunset stage. Reverting is not a solution here. If the main problem is only related to action="error" we can ditch the deprecated decorator and use AirflowException.

As you suggested, we can raise the AirflowException

we are already raising this for other operators with the help of the method: _raise_exception_for_deprecated_operator)

for the following classes: AutoMLTablesUpdateDatasetOperator and AutoMLDeployModelOperator .

If this is OK, I will try to prepare a new PR which include this change

@Taragolis
Copy link
Contributor Author

@molcay yeah sure, if you could fix it it would be nice, after that I will add separate check in pre-commit for prevent use action keyword in @deprecated.

I've created revert only for the simple way to fix because I do not hace any idea what should be fixed and which service no longer provided by Google/GCP/etc

Let me move it to draft for now, for prevent accidental merge

@Taragolis Taragolis marked this pull request as draft May 22, 2024 15:37
@shahar1
Copy link
Contributor

shahar1 commented May 22, 2024

@Taragolis Thanks for pointing it out; I wasn't aware of this matter while reviewing previous PR.
@molcay I'd be happy to review your fix :)

@molcay
Copy link
Contributor

molcay commented May 23, 2024

Hi @Taragolis,

We realized that we can do this change within a PR (#39752) that is already created for the community.

@e-galan will be sending already sent the changes accordingly for #39752 and we can merge that PR to fix all of the action="error" misuse (2 operators from this PR: AutoMLTablesUpdateDatasetOperator, AutoMLDeployModelOperator and 2 operators from #39752).

@shahar1 you can review #39752

@eladkal if you can follow the #39752 for releasing the next provider version for apache-airflow-providers-google.

@eladkal
Copy link
Contributor

eladkal commented May 25, 2024

Now that #39752 is merged. Can we close this PR?

@molcay
Copy link
Contributor

molcay commented May 27, 2024

@eladkal Yes, we can close this PR.

@eladkal eladkal closed this May 27, 2024
@eladkal eladkal deleted the revert-38673-deprecate-automl branch May 27, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants