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

Test standard provider with Airflow 2.8 and 2.9 #43556

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Oct 31, 2024

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
Member Author

potiuk commented Oct 31, 2024

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

@gopidesupavan
Copy link
Contributor

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
Member Author

potiuk commented Oct 31, 2024

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 potiuk force-pushed the test-standard-provider-with-pre-2-10 branch 7 times, most recently from d226946 to 4e2c7e8 Compare November 3, 2024 10:55
@potiuk potiuk force-pushed the test-standard-provider-with-pre-2-10 branch 3 times, most recently from 1a558cc to 630eb6f Compare November 3, 2024 16:42
@potiuk potiuk force-pushed the test-standard-provider-with-pre-2-10 branch from 630eb6f to 7e8159a Compare November 3, 2024 18:07
@potiuk
Copy link
Member Author

potiuk commented Nov 3, 2024

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

@potiuk
Copy link
Member Author

potiuk commented Nov 3, 2024

The fix here #43617 allows to iterate fast on the "main providers" + "old airflow" case :)

@potiuk potiuk force-pushed the test-standard-provider-with-pre-2-10 branch 2 times, most recently from c76dcf9 to 2f7a44f Compare November 4, 2024 17:45
@potiuk potiuk force-pushed the test-standard-provider-with-pre-2-10 branch 2 times, most recently from 99ad6f7 to 77e9f92 Compare November 4, 2024 21:31
@potiuk potiuk force-pushed the test-standard-provider-with-pre-2-10 branch 2 times, most recently from 7021195 to 6ad6365 Compare November 6, 2024 17:44
@@ -102,7 +119,9 @@ def __init__(

def execute(self, context: Context) -> NoReturn:
self.defer(
trigger=DateTimeTrigger(moment=self.target_datetime, end_from_trigger=self.end_from_trigger),
trigger=DateTimeTrigger(moment=self.target_datetime, end_from_trigger=self.end_from_trigger)
if AIRFLOW_V_3_0_PLUS
Copy link
Member

Choose a reason for hiding this comment

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

I thought it's 2_10_PLUS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me try

@potiuk potiuk force-pushed the test-standard-provider-with-pre-2-10 branch from 6ad6365 to 5e2c180 Compare November 7, 2024 09:42
@gopidesupavan
Copy link
Contributor

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
Member Author

potiuk commented Nov 8, 2024

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
Member Author

potiuk commented Nov 8, 2024

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
Contributor

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
Contributor

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

@potiuk
Copy link
Member Author

potiuk commented Nov 8, 2024

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

I am fixing it :)

@potiuk potiuk force-pushed the test-standard-provider-with-pre-2-10 branch 2 times, most recently from f15c40e to 1e25069 Compare November 8, 2024 01:36
@potiuk
Copy link
Member Author

potiuk commented Nov 8, 2024

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
Contributor

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

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 potiuk force-pushed the test-standard-provider-with-pre-2-10 branch from bf8f2e6 to 34789e6 Compare November 9, 2024 00:06
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