Skip to content

Test standard provider with Airflow 2.8 and 2.9 - #43556

Merged
potiuk merged 1 commit into
apache:mainfrom
potiuk:test-standard-provider-with-pre-2-10
Nov 9, 2024
Merged

Test standard provider with Airflow 2.8 and 2.9#43556
potiuk merged 1 commit into
apache:mainfrom
potiuk:test-standard-provider-with-pre-2-10

Conversation

@potiuk

Copy link
Copy Markdown
Member

The standard provider has now min version of Airflow = 2.8 since #43553, but we have not tested it for Airflow 2.8 and 2.9.


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

@potiuk

Copy link
Copy Markdown
MemberAuthor

Ha.... so we are NOT good with standard provider for 2.8 and 2.9 YET

@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch from ab92765 to 4ce61b3CompareOctober 31, 2024 23:00
@gopidesupavan

Copy link
Copy Markdown
Member

Ha.... so we are NOT good with standard provider for 2.8 and 2.9 YET

hm sorry my bad 😞 , i thought only on airflow 2.10

@potiuk

Copy link
Copy Markdown
MemberAuthor

Ha.... so we are NOT good with standard provider for 2.8 and 2.9 YET

hm sorry my bad 😞 , i thought only on airflow 2.10

No worries :). Nice excercise to fix it for 2.8 and 2.9 :)

@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch 7 times, most recently from d226946 to 4e2c7e8CompareNovember 3, 2024 10:55
Comment threadproviders/src/airflow/providers/standard/operators/python.py
@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch 3 times, most recently from 1a558cc to 630eb6fCompareNovember 3, 2024 16:42
@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch from 630eb6f to 7e8159aCompareNovember 3, 2024 18:07
@potiuk

Copy link
Copy Markdown
MemberAuthor

There are still some interesting problems to solve :). I already found another teething problem from providers move :)

@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch from 99ad6f7 to 77e9f92CompareNovember 4, 2024 21:31
@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch 2 times, most recently from 7021195 to 6ad6365CompareNovember 6, 2024 17:44
Comment threadproviders/src/airflow/providers/standard/sensors/time.py Outdated
@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch from 6ad6365 to 5e2c180CompareNovember 7, 2024 09:42
@gopidesupavan

Copy link
Copy Markdown
Member

alright got it where the failures coming from for this providers/tests/standard/operators/test_python.py::TestBranchExternalPythonOperator::test_use_airflow_context_touch_other_variables its acutally difference in the deserialisation between 2.8 and 2.10

2.10 has this casting Context: https://github.com/apache/airflow/blob/v2-10-stable/airflow/serialization/serialized_objects.py#L829, where as 2.8 and 2.9 doesn't have this implementation.

@potiuk

Copy link
Copy Markdown
MemberAuthor

alright got it where the failures coming from for this providers/tests/standard/operators/test_python.py::TestBranchExternalPythonOperator::test_use_airflow_context_touch_other_variables its acutally difference in the deserialisation between 2.8 and 2.10

Nice. Good job. So now we have to find out how this deserialization happened in 2.8/2.9 PythonVirtualenv. Let me take a look

@potiuk

Copy link
Copy Markdown
MemberAuthor

AH... Of course. Sending context was added in 3.0.0 :) . So the fact that it actually works on 2.10 is a bonus of new standard providers, but there should be no expectation it should work on 2.8 or 2.9 because it never worked there!

@gopidesupavan

Copy link
Copy Markdown
Member

AH... Of course. Sending context was added in 3.0.0 :) . So the fact that it actually works on 2.10 is a bonus of new standard providers, but there should be no expectation it should work on 2.8 or 2.9 because it never worked there!

Yeah that's correct, should we make necessary changes to support it only for 2.10+

@gopidesupavan

Copy link
Copy Markdown
Member

Happy to make changes, I believe only need some test fixes and use_airflow_context
Parameter checks between Versions

@potiuk

Copy link
Copy Markdown
MemberAuthor

Happy to make changes, I believe only need some test fixes and use_airflow_context Parameter checks between Versions

I am fixing it :)

@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch 2 times, most recently from f15c40e to 1e25069CompareNovember 8, 2024 01:36
@potiuk

Copy link
Copy Markdown
MemberAuthor

Actually - really the thing is that that it was only working on 2.10 when ENABLE_AIP_44 was used, so we really should only enable it only for Airlfow 3+

@gopidesupavan

Copy link
Copy Markdown
Member

Actually - really the thing is that that it was only working on 2.10 when ENABLE_AIP_44 was used, so we really should only enable it only for Airlfow 3+

Yes your correct. for airflow less thank 3 versions we can disable, currently by default its coming as true for all the versions.

I ran this in local ,
breeze shell --use-airflow-version 2.8.4 --mount-sources providers-and-tests and in the tests its coming as true.

raised change here: #43818

@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch 2 times, most recently from bf8f2e6 to 34789e6CompareNovember 9, 2024 00:06
The standard provider has now min version of Airflow = 2.8
since apache#43553, but we have not tested it for Airflow 2.8 and 2.9.
@potiuk
potiukforce-pushed the test-standard-provider-with-pre-2-10 branch from 34789e6 to b8c297dCompareNovember 9, 2024 08:29
@potiuk
potiuk merged commit 9bbf6f1 into apache:mainNov 9, 2024
@potiuk
potiuk deleted the test-standard-provider-with-pre-2-10 branch November 9, 2024 16:40
@potiuk

Copy link
Copy Markdown
MemberAuthor

@gopidesupavan - you can rebase and check yout #43818 now

@gopidesupavan

Copy link
Copy Markdown
Member

@gopidesupavan - you can rebase and check yout #43818 now

woohoo merged thank you @potiuk :) . Let me rebase other pr.

ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
The standard provider has now min version of Airflow = 2.8
since apache#43553, but we have not tested it for Airflow 2.8 and 2.9.
Comment on lines +18 to +26
from __future__ import annotations

from packaging.version import Version

from airflow import __version__ as airflow_version

AIRFLOW_VERSION = Version(airflow_version)
AIRFLOW_V_2_10_PLUS = Version(AIRFLOW_VERSION.base_version) >= Version("2.10.0")
AIRFLOW_V_3_0_PLUS = Version(AIRFLOW_VERSION.base_version) >= Version("3.0.0")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will be overridden during release time as the provider __init__ is generated from template.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Raised #44017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

aaaaaa! !

Sign up for freeto 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.

5 participants

@potiuk@gopidesupavan@Lee-W@hussein-awala@eladkal