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

Standard provider python operator #42081

Merged

Conversation

gopidesupavan
Copy link
Member

Moving python operators/sensors to standard provider


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

@gopidesupavan
Copy link
Member Author

cc: @romsharon98

@gopidesupavan gopidesupavan force-pushed the standard-provider-python-operator branch from e873eaa to 58d1db9 Compare October 5, 2024 09:24
@gopidesupavan gopidesupavan force-pushed the standard-provider-python-operator branch from 5f2d337 to 3cbf146 Compare October 12, 2024 13:23
@gopidesupavan
Copy link
Member Author

Some of the tests are failing due to compatibility issues. It appears that the changes related to the triggering_asset_events event are associated with Airflow 3.0.0. Since we're transitioning the Python operator into the providers' ecosystem, the standard provider now requires a minimum Airflow version of 2.10.0. As a result, the compatibility tests for version 2.10.1 are failing. Any suggestions ?

https://github.com/apache/airflow/actions/runs/11308098417/job/31450685352?pr=42081#step:11:12372

@gopidesupavan
Copy link
Member Author

@potiuk It appears that this PR needs to be initiated from the Apache repository, or it should be someone with committer privileges. The PR involves modifications in the dev/breeze and dev/tests_common/test_utils/compat.py sections, but it seems that the CI process is overriding these scripts with those from the main branch, which could be causing some issues.

Here it is removing https://github.com/apache/airflow/actions/runs/11308098408/job/31450591466#step:4:219

https://github.com/apache/airflow/pull/42081/files#diff-cbadf25516149c7fe3d19e45b5b03789f4bb93bd6d837eaa365dfd0ffb771a0fR55

For dev/tests_common/test_utils/compat.py have workaround but dont like this, i have imported operators with try/catch like this https://github.com/apache/airflow/pull/42081/files#diff-a842cdd5bcf15ce8859d6115b48853c3ac654701a3279bce73969be8922dbbe8R47. these imports can be replaced using compat.

Any thoughts please ?

@gopidesupavan gopidesupavan marked this pull request as ready for review October 13, 2024 01:23
@gopidesupavan
Copy link
Member Author

@potiuk It appears that this PR needs to be initiated from the Apache repository, or it should be someone with committer privileges. The PR involves modifications in the dev/breeze and dev/tests_common/test_utils/compat.py sections, but it seems that the CI process is overriding these scripts with those from the main branch, which could be causing some issues.

Here it is removing https://github.com/apache/airflow/actions/runs/11308098408/job/31450591466#step:4:219

https://github.com/apache/airflow/pull/42081/files#diff-cbadf25516149c7fe3d19e45b5b03789f4bb93bd6d837eaa365dfd0ffb771a0fR55

For dev/tests_common/test_utils/compat.py have workaround but dont like this, i have imported operators with try/catch like this https://github.com/apache/airflow/pull/42081/files#diff-a842cdd5bcf15ce8859d6115b48853c3ac654701a3279bce73969be8922dbbe8R47. these imports can be replaced using compat.

Any thoughts please ?

looks like the tests are going through fine now.

@potiuk It appears that this PR needs to be initiated from the Apache repository, or it should be someone with committer privileges. The PR involves modifications in the dev/breeze and dev/tests_common/test_utils/compat.py sections, but it seems that the CI process is overriding these scripts with those from the main branch, which could be causing some issues.

Here it is removing https://github.com/apache/airflow/actions/runs/11308098408/job/31450591466#step:4:219

https://github.com/apache/airflow/pull/42081/files#diff-cbadf25516149c7fe3d19e45b5b03789f4bb93bd6d837eaa365dfd0ffb771a0fR55

For dev/tests_common/test_utils/compat.py have workaround but dont like this, i have imported operators with try/catch like this https://github.com/apache/airflow/pull/42081/files#diff-a842cdd5bcf15ce8859d6115b48853c3ac654701a3279bce73969be8922dbbe8R47. these imports can be replaced using compat.

Any thoughts please ?

looks like tests are running..now..

@gopidesupavan
Copy link
Member Author

Compat tests 2.10.1 failing because python operator has changes related to airflow 3.0.0, should i update back changes to support in 2.10.X?

@potiuk
Copy link
Member

potiuk commented Oct 13, 2024

Compat tests 2.10.1 failing because python operator has changes related to airflow 3.0.0, should i update back changes to support in 2.10.X?

It looks like it's more of "unit test compatibility" than backporting it. I think in this case simply tests should be fixed to be able to handle past providers. We do not need to backport this change, but we need to make sure that the assertions work when airflow is installed from "2.10.1"

@gopidesupavan
Copy link
Member Author

Compat tests 2.10.1 failing because python operator has changes related to airflow 3.0.0, should i update back changes to support in 2.10.X?

It looks like it's more of "unit test compatibility" than backporting it. I think in this case simply tests should be fixed to be able to handle past providers. We do not need to backport this change, but we need to make sure that the assertions work when airflow is installed from "2.10.1"

thanks jarek, When installing Airflow 2.10.1, the Python operator is included as part of the core package. Should assertions be checked against the core Python operator (i.e., airflow.operators.python) or against airflow.providers.standard.operators.python? Opting for the former would require code changes due to the adjustments already made for Airflow 3. Do you agree that if Airflow 2.10.1 is installed, validation tests should be conducted on airflow.operators.python, as long as the tests remain compatible?

@gopidesupavan
Copy link
Member Author

Compat tests 2.10.1 failing because python operator has changes related to airflow 3.0.0, should i update back changes to support in 2.10.X?

It looks like it's more of "unit test compatibility" than backporting it. I think in this case simply tests should be fixed to be able to handle past providers. We do not need to backport this change, but we need to make sure that the assertions work when airflow is installed from "2.10.1"

thanks jarek, When installing Airflow 2.10.1, the Python operator is included as part of the core package. Should assertions be checked against the core Python operator (i.e., airflow.operators.python) or against airflow.providers.standard.operators.python? Opting for the former would require code changes due to the adjustments already made for Airflow 3. Do you agree that if Airflow 2.10.1 is installed, validation tests should be conducted on airflow.operators.python, as long as the tests remain compatible?

ah i got it what you mean by 'assertions work', need to make some adjustments in the python operator tests related to new feature updates. hope that works..

@potiuk
Copy link
Member

potiuk commented Oct 14, 2024

ah i got it what you mean by 'assertions work', need to make some adjustments in the python operator tests related to new feature updates. hope that works..

Yes. Some examples and detailed explanation on how to do this "test_compatibility" are here: https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#implementing-compatibility-for-provider-tests-for-older-airflow-versions

@gopidesupavan gopidesupavan force-pushed the standard-provider-python-operator branch from 2db922d to 3e743df Compare October 31, 2024 02:55
@gopidesupavan
Copy link
Member Author

One failure related to spell check this will fix #43538

@gopidesupavan
Copy link
Member Author

Are we good to merge this ? 😄

@potiuk
Copy link
Member

potiuk commented Oct 31, 2024

Go ahead and do the honors @gopidesupavan -> That's an interesting one as first(?) serious one to merge as a committer.. Don't be shy. We can always revert it if it breaks things ;)

@gopidesupavan gopidesupavan merged commit 06088a3 into apache:main Oct 31, 2024
105 of 106 checks passed
@gopidesupavan
Copy link
Member Author

Go ahead and do the honors @gopidesupavan -> That's an interesting one as first(?) serious one to merge as a committer.. Don't be shy. We can always revert it if it breaks things ;)

woohoooo merged thank you all for the help and review here, really appreciate it 😄

ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
* add core.time provider

* add source-date-epoch

* change core to essentials

* revert external task sensor location

* add provider to airflow_providers_bug_report list

* change new provider name to standard

* add integration

* revert hatch_build

* move examples back to airflow core

* change sensors example dags paths

* remove init

* revert howto docs

* change provider as not-ready

* Move python operator to standard provider

* fix test failures

* move python operator inside standard/operators

* rebase and update python imports

* keep standard provider in hatch_build

* update selective checks and dependencies

* update imports and dependency file

* fix compat import errors

* fix selective_checks and imports from compat

* remove imports from compat for pythonoperator

* update run_tests.py python branching test paths

* add setup method in python operator tests

* check imports in python operator tests

* fix imports and move python virtual env scripts inside standard provider

* update selective checks tests and imports in test_python

* move python_virtualenv tests inside standard provider

* fix pre-commit static checks

* use compat provider for standard provider imports

* fix _SERIALIZERS ModuleNotFoundError and static checks

* import _SERIALIZERS for AIRFLOW_2_10 or higher versions

* fix ruff format

* fix static checks in test_dag

* fix test_provide_context_does_not_fail failure: add TypeError to pytest assert

---------

Co-authored-by: romsharon98 <[email protected]>
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.

7 participants