-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
base: main
Are you sure you want to change the base?
Conversation
Ha.... so we are NOT good with standard provider for 2.8 and 2.9 YET |
ab92765
to
4ce61b3
Compare
hm sorry my bad 😞 , i thought only on airflow 2.10 |
No worries :). Nice excercise to fix it for 2.8 and 2.9 :) |
d226946
to
4e2c7e8
Compare
1a558cc
to
630eb6f
Compare
630eb6f
to
7e8159a
Compare
There are still some interesting problems to solve :). I already found another teething problem from providers move :) |
The fix here #43617 allows to iterate fast on the "main providers" + "old airflow" case :) |
c76dcf9
to
2f7a44f
Compare
99ad6f7
to
77e9f92
Compare
7021195
to
6ad6365
Compare
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try
6ad6365
to
5e2c180
Compare
alright got it where the failures coming from for this 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. |
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 |
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+ |
Happy to make changes, I believe only need some test fixes and use_airflow_context |
I am fixing it :) |
f15c40e
to
1e25069
Compare
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+ |
1e25069
to
de13808
Compare
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 , raised change here: #43818 |
de13808
to
bf8f2e6
Compare
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.
bf8f2e6
to
34789e6
Compare
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.