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

Move BaseOperatorLink into the separate module #35032

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

Taragolis
Copy link
Contributor

Move BaseOperatorLink from airflow.models.baseoperator into the separate module this might slightly reduce import in case of creation extra.

In additional remove check-base-operator-usage pre-commit checks which are prevents direct use of airflow.models.baseoperator.BaseOperatorLink and forced contributors to use airflow.models.BaseOperatorLink in providers and other parts


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

@Taragolis Taragolis added the full tests needed We need to run full set of tests for this PR to merge label Oct 18, 2023
@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:core-operators Operators, Sensors and hooks within Core Airflow area:dev-tools area:serialization area:webserver Webserver related Issues kind:documentation labels Oct 18, 2023
@potiuk
Copy link
Member

potiuk commented Oct 18, 2023

The new packages should be added here: https://airflow.apache.org/docs/apache-airflow/stable/public-airflow-interface.html

Also - are we going to update the providers with importing the new one ? Otherwise all operators will raise deprecation warning ?

@Taragolis
Copy link
Contributor Author

Community providers should not raise warnings, because all of them use lazy import from airflow.models

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. @uranusjr ?

@potiuk
Copy link
Member

potiuk commented Oct 19, 2023

Community providers should not raise warnings, because all of them use lazy import from airflow.models

Right. Then we don't even have to do the try/Except. BUT that made me think. Maybe this calls for NOT removing the provider pre-commit @Taragolis ? There is a reason why it is like that and if we remove the pre-commit, then providers MIGHT start diverging where they are importing it from.

WDYT?

@Taragolis
Copy link
Contributor Author

Then we don't even have to do the try/Except.

Try/except might required only if #34600 completed. Some ideas why it might be a good idea (or maybe not)

There is a reason why it is like that and if we remove the pre-commit, then providers MIGHT start diverging where they are importing it from.

We could ban airflow.models.baseoperator.BaseOperatorLink in airflow codebase by the same way as we ban
from airflow import AirflowException and from airflow import AirflowException

airflow/pyproject.toml

Lines 151 to 153 in 303a329

[tool.ruff.flake8-tidy-imports.banned-api]
"airflow.AirflowException".msg = "Use airflow.exceptions.AirflowException instead."
"airflow.Dataset".msg = "Use airflow.datasets.Dataset instead."

@potiuk
Copy link
Member

potiuk commented Oct 19, 2023

We could ban airflow.models.baseoperator.BaseOperatorLink in airflow codebase by the same way as we ban
from airflow import AirflowException and from airflow import AirflowException

Yep. We could. That would work.

@@ -569,31 +569,6 @@ repos:
entry: ./scripts/ci/pre_commit/pre_commit_sync_init_decorator.py
pass_filenames: false
files: ^airflow/models/dag\.py$|^airflow/(?:decorators|utils)/task_group\.py$
- id: check-base-operator-usage
Copy link
Member

@uranusjr uranusjr Oct 20, 2023

Choose a reason for hiding this comment

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

I’m not sure if this should be removed outright. Instead we may want to check the two classes are imported from respectively correct modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for clarify which behavior we expect here

Option 1:
Outside of core airflow.models.baseoperator.BaseOperator and airflow.models.baseoperatorlink.BaseOperatorLink should be imported from airflow.models

Option 2:
Allow to use airflow.models.baseoperator.BaseOperator and airflow.models.baseoperatorlink.BaseOperatorLink
Ban import airflow.models.baseoperator.BaseOperatorLink

Copy link
Member

Choose a reason for hiding this comment

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

In core: Allow only airflow.models.baseoperator.BaseOperator and airflow.models.baseoperatorlink.BaseOperatorLink

Outside core: Allow only airflow.models.BaseOperator and airflow.models.BaseOperatorLink

Copy link
Member

Choose a reason for hiding this comment

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

yeah.

Copy link
Contributor

Choose a reason for hiding this comment

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

@uranusjr - Are you good with this one getting merged?

@eladkal eladkal added this to the Airflow 2.8.0 milestone Oct 28, 2023
@eladkal
Copy link
Contributor

eladkal commented Nov 3, 2023

static checks are failing

@ferruzzi
Copy link
Contributor

It's all green and Jarek has approved. No reply from Tzu-ping in a few weeks but it does appear to me that his concerns were addressed. I think we can merge it??

@Taragolis
Copy link
Contributor Author

Let's rebase first and have a look is is still working

@Taragolis Taragolis merged commit b1a9ebb into apache:main Nov 21, 2023
72 checks passed
@Taragolis Taragolis deleted the move-baseoperatorlink branch November 21, 2023 17:10
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Nov 23, 2023
ephraimbuddy pushed a commit that referenced this pull request Nov 23, 2023
* Move `BaseOperatorLink` into the separate module

* Add `airflow.models.baseoperatorlink` as part of Public Interface of Airflow

* Ban `airflow.models.baseoperator.BaseOperatorLink` usage in codebase

* Return back check-base-operator-usage pre-commit hooks
ephraimbuddy pushed a commit that referenced this pull request Nov 26, 2023
* Move `BaseOperatorLink` into the separate module

* Add `airflow.models.baseoperatorlink` as part of Public Interface of Airflow

* Ban `airflow.models.baseoperator.BaseOperatorLink` usage in codebase

* Return back check-base-operator-usage pre-commit hooks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:core-operators Operators, Sensors and hooks within Core Airflow area:dev-tools area:serialization area:webserver Webserver related Issues full tests needed We need to run full set of tests for this PR to merge kind:documentation type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants