-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add Pushing XComs from Deferred example
#49535
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
Add Pushing XComs from Deferred example
#49535
Conversation
Co-Authored-By: Avyukt Soni <95626105+avyuktsoni0731@users.noreply.github.com>
| Pushing XComs from Deferred Tasks | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| .. versionadded:: 2.10.0 |
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.
Do we have an easy way to generate a list of all our Trigger that were written before 2.10 and don't support this? so we can patch them? in ~1 week or so the min version for providers will be 2.10 so we can safely fix it.
Also maybe, an idea how to get the list... adding a test to make sure any sub class of BaseTrigger has implementation for this? That would also protect us for future adding new triggers
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.
Sorry for the late. It took some time to deal with this. And I think this func is not supported for all triggers before 2.10.0 since there are not implementation of some thing like self.xcoms or something like the example do.
I only see the trigger test which use the functionality the example show
airflow/airflow-core/tests/unit/models/test_trigger.py
Lines 202 to 211 in c37e6eb
| @pytest.mark.parametrize( | |
| "event_cls, expected", | |
| [ | |
| (TaskSuccessEvent, "success"), | |
| (TaskFailedEvent, "failed"), | |
| (TaskSkippedEvent, "skipped"), | |
| ], | |
| ) | |
| @patch("airflow.utils.timezone.utcnow") | |
| def test_submit_event_task_end(mock_utcnow, session, create_task_instance, event_cls, expected): |
please let me know if there is anything I could help
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.
This is my point. Minimum version for Airflow in providers is 2.10 so we should open a tracking issue to modify all trigger we have to support it. Don't we?
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.
After more investigation, it seems currently trigger is not accept xcoms #46677 (comment) after reworked. xcoms is passed when the event have xcoms.
Should we modify our example or try to modify trigger to accept "xcoms" when init?
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 think we could leave this here until there is any re-implementation plan.
Pushing XComs from Deferred examplePushing XComs from Deferred example
Related Issue
closes #44759
Why
How
cc: @eladkal @tirkarthi @avyuktsoni0731
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.