airflow icon indicating copy to clipboard operation
airflow copied to clipboard

Standard provider python operator

Open gopidesupavan opened this issue 1 year ago • 12 comments

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 avatar Sep 07 '24 13:09 gopidesupavan

cc: @romsharon98

gopidesupavan avatar Sep 15 '24 10:09 gopidesupavan

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 avatar Oct 12 '24 20:10 gopidesupavan

@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 avatar Oct 12 '24 20:10 gopidesupavan

@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 avatar Oct 13 '24 02:10 gopidesupavan

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?

gopidesupavan avatar Oct 13 '24 09:10 gopidesupavan

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"

potiuk avatar Oct 13 '24 16:10 potiuk

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 avatar Oct 13 '24 18:10 gopidesupavan

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

gopidesupavan avatar Oct 13 '24 19:10 gopidesupavan

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

potiuk avatar Oct 14 '24 01:10 potiuk

I believe we have to move these scripts as well into standard provider airflow/utils/python_virtualenv.py airflow/utils/python_virtualenv_script.jinja2 ?

gopidesupavan avatar Oct 15 '24 22:10 gopidesupavan

I believe we have to move these scripts as well into standard provider airflow/utils/python_virtualenv.py airflow/utils/python_virtualenv_script.jinja2 ?

Yes. Absolutely.

potiuk avatar Oct 16 '24 07:10 potiuk

I believe we have to move these scripts as well into standard provider airflow/utils/python_virtualenv.py airflow/utils/python_virtualenv_script.jinja2 ?

Yes. Absolutely.

cool, Thank you :)

gopidesupavan avatar Oct 16 '24 08:10 gopidesupavan

Just one thing about compat provider reuse.

yeah am updating imports to use compat.

gopidesupavan avatar Oct 28 '24 09:10 gopidesupavan

Just one thing about compat provider reuse.

yeah am updating imports to use compat.

I've added the imports to the compat provider, but I've noticed test failures occurring in selective checks. This is likely because all provider imports are now referenced directly from the compat provider. So, when we revert to standard provider direct imports, we may need to update all the selective check tests accordingly.

image

Is it worth adding to compat provider now?

gopidesupavan avatar Oct 28 '24 13:10 gopidesupavan

Just one thing about compat provider reuse.

yeah am updating imports to use compat.

I've added the imports to the compat provider, but I've noticed test failures occurring in selective checks. This is likely because all provider imports are now referenced directly from the compat provider. So, when we revert to standard provider direct imports, we may need to update all the selective check tests accordingly.

No - it's ok - you need to fix the tests accordingly. Those tests are testing whether the right set of "dependent" providers are selected when selective checks is running - so when you start depending on the standard provider and when standard provider depends on some other provider., the tests start to expect changes in the depednencies, so what needs to be done is to update the tests to reflect that.

potiuk avatar Oct 28 '24 16:10 potiuk

And it's not that difficult, most of the code is common in constants, and some other could be replaced with search/replace.

potiuk avatar Oct 28 '24 16:10 potiuk

Just one thing about compat provider reuse.

yeah am updating imports to use compat.

I've added the imports to the compat provider, but I've noticed test failures occurring in selective checks. This is likely because all provider imports are now referenced directly from the compat provider. So, when we revert to standard provider direct imports, we may need to update all the selective check tests accordingly.

No - it's ok - you need to fix the tests accordingly. Those tests are testing whether the right set of "dependent" providers are selected when selective checks is running - so when you start depending on the standard provider and when standard provider depends on some other provider., the tests start to expect changes in the depednencies, so what needs to be done is to update the tests to reflect that.

Sure , thanks jarek. Will push the changes..

gopidesupavan avatar Oct 28 '24 16:10 gopidesupavan

BTW. ... some conflicts to solve - so you need to rebase too

potiuk avatar Oct 28 '24 17:10 potiuk

BTW. ... some conflicts to solve - so you need to rebase too

Thanks jarek, finally this is green now.

gopidesupavan avatar Oct 29 '24 01:10 gopidesupavan

One failure related to spell check this will fix https://github.com/apache/airflow/pull/43538

gopidesupavan avatar Oct 31 '24 09:10 gopidesupavan

Are we good to merge this ? 😄

gopidesupavan avatar Oct 31 '24 09:10 gopidesupavan

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 ;)

potiuk avatar Oct 31 '24 10:10 potiuk

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 😄

gopidesupavan avatar Oct 31 '24 10:10 gopidesupavan