fix Inconsistent XCom Return Type in Mapped Task Groups with Dynamic …#59104
fix Inconsistent XCom Return Type in Mapped Task Groups with Dynamic …#59104henry3260 wants to merge 5 commits intoapache:mainfrom
Conversation
b70e671 to
16596fb
Compare
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the PR!
It would be nice to add the corresponding unit tests as well, thanks!
jason810496
left a comment
There was a problem hiding this comment.
Sorry for the delayed review, thanks for the fix!
LGTM overall, but we still need some refactor on the test to verify the expected behavior, thanks!
5631f70 to
d11f843
Compare
|
@jason810496 @uranusjr I've addressed the comments. Please let me know if there's anything else blocking the merge. |
jason810496
left a comment
There was a problem hiding this comment.
LGTM. Not sure if @uranusjr has any additional comments.
|
Hi all, we hit the same issue with Airflow 3.1.7. I don’t think the changes in this PR are sufficient, though. In RuntimeTaskInstance there’s also a check that returns a single item instead of the list when the XCom list has only one element (see https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/execution_time/task_runner.py#L392). Shouldn’t that be updated too? |
|
Oh, this is stale since a while but something I'd expected basic. @uranusjr can you re-review? Not sure the other problem in task_runner.py as mentioned above can also be a spearate PR not to delay here further. But see the same inconsistency there as well and needs to be fixed. |
yeah, maybe I can open another one to address inconsistent issue mentioned abbove. |
@uranusjr could you re-review? |
|
@henry3260 can you adjust to get it into 3.1.8 as a fix? |
Yeah @henry3260, planning to cut 3.1.8rc1 soon, appreciate it if you can implement the latest review comments |
OK, I will take a look! |
3490f4f to
c026e6a
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses inconsistent TaskInstance.xcom_pull() return types for mapped tasks by returning a LazyXComSelectSequence when pulling mapped XComs (map_index >= 0) without explicitly specifying map_indexes, and adds unit tests to validate mapped vs. unmapped behavior.
Changes:
- Update
TaskInstance.xcom_pull()to returnLazyXComSelectSequencefor mapped XComs whenmap_indexesis not specified. - Add unit tests asserting unmapped pulls return a single deserialized value and mapped pulls return a lazy sequence.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
airflow-core/src/airflow/models/taskinstance.py |
Adjusts xcom_pull to consistently return a lazy sequence for mapped XComs in the single-task pull path. |
airflow-core/tests/unit/models/test_taskinstance.py |
Adds tests covering unmapped vs. mapped xcom_pull return types. |
closes: #59008
^ 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.