-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Add ability to pull XCom when passing multiple map_indexes #46728
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
Conversation
|
I am still not sure if this is the intended implementation or not, but it is limited by the current execution API restriction of 1 XCom result in response. I didn't touch it to not break anything. Prints from original DAG example: @dag.task
def xcom_pull(**context):
ti: TaskInstance = context["ti"]
# [1, 3, None, None, None, None]
for x in [
ti.xcom_pull(task_ids=["identity", "foo", "plusfive"], map_indexes=[0, 2]),
]:
print(x) |
|
If this PR is not in the right direction I am ok with full rework. I just need to understand what was planned for this feature then. |
42bea72 to
dbb0021
Compare
|
Hello @insomnes, apologies for the delayed review. Taking a look at it. |
amoghrajesh
left a comment
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.
Initial set of comments, WDYT @ashb?
|
@amoghrajesh thanks for the review! No worries about the delay! |
2fdc2a9 to
6a31d6d
Compare
|
Up. Would be nice to know what's next. |
80488d5 to
f73609f
Compare
|
Up. If this PR should wait for something else or other solution should be chosen, than it would be nice to have some update about it. |
a2a9c84 to
d8d4a15
Compare
521d251 to
fa61af3
Compare
fa61af3 to
66e6dfe
Compare
kaxil
left a comment
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.
At some point later, we should just maybe expand the API endpoint to optionally get results for multiple map indexes, fine for now!
amoghrajesh
left a comment
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.
Thanks for the patience @insomnes, its taken long to get a review here 🗡️
Few nits, preemptively approving once those are handled.
7a21a50 to
b4c1de8
Compare
|
Thank you @kaxil and @amoghrajesh for the review.
No problem! I just got a bit lost on the best way to work around the current restrictions of the API, because I don't have a full picture. I've applied the suggestions as is so I resolved the comments for them |
12aee79 to
282ab87
Compare
|
I've resolved two comments, they were properly fixed, I believe. Still left the thread with the API limitations discussion unresolved, because I am unsure if it is appropriate to do it myself in this case. Other threads were simple fixes. |
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
282ab87 to
6fbf1ff
Compare
|
Rebased on recent changes to |
|
@insomnes i have rebased it again, once green, will merge, thanks for your patience |
Thank you, checks are green now. |
) * Add ability to pull XCom when passing multiple map_indexes * Add comment about LazyXComSequence and orignal description * Apply review comments * Update task_sdk/src/airflow/sdk/execution_time/task_runner.py Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> * Fix review comments --------- Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
) * Add ability to pull XCom when passing multiple map_indexes * Add comment about LazyXComSequence and orignal description * Apply review comments * Update task_sdk/src/airflow/sdk/execution_time/task_runner.py Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> * Fix review comments --------- Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
) * Add ability to pull XCom when passing multiple map_indexes * Add comment about LazyXComSequence and orignal description * Apply review comments * Update task_sdk/src/airflow/sdk/execution_time/task_runner.py Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> * Fix review comments --------- Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com> Co-authored-by: Amogh Desai <amoghrajesh1999@gmail.com>
This PR adds the ability to pull XCom when passing multiple
map_indexesonRuntimeTaskInstance.Nonecase for task idcloses: #46665
^ 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 newsfragments.