-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Refactor out xcom constants from models #30180
Conversation
606c35b
to
7ac2de9
Compare
airflow/models/xcom.py
Outdated
from airflow.utils.xcom import ( | ||
MAX_XCOM_SIZE, # noqa: F401 (#30180 - leaving it for providers backward compatibility) | ||
XCOM_RETURN_KEY, | ||
) |
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.
When I tried to migrate the providers' code to import airflow.utils.xcom
, the Airflow 2.3 compatibility test for them failed (https://github.com/apache/airflow/actions/runs/4456673101/jobs/7827374562?pr=30180) since naturally there was no corresponding package.
I'm not sure what the right way to migrate them is, would appreciate some guidance!
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.
Figure out a way.
For exampele import the new constants in the old place and mark it with comments to remove it when min-airflow-version is 2.6.
The test is right (and was specifically designed to inform and prevent you from ignoribng the fact that new providers can be used with old airflow (and old providers with new airflow).
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.
The test is right
Yeah, sure, I didn't have any doubts around that :)
For exampele import the new constants in the old place and mark it with comments to remove it when min-airflow-version is 2.6.
Makes sense to me, I think this is exactly what I came up with, I'll extend the comment with the minimal airflow version according to your proposal then, thanks!
a563168
to
b6455f1
Compare
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.
That looks good now :)
Sometimes it may be needed to use the constants to have a single source of truth, but we may not want to introduce a dependency on the models layer, where it's possible to avoid that (e.g., in CLI parsing - #29608 (comment)).
Note: Creating it as a separate PR to keep the above mentioned one more focused on the specific
--xcoms
CLI arg task and facilitate the review.