Repository navigation
Fix xcom_pull() ignoring default parameter when map_indexes not set - #64614
nagasrisai wants to merge 6 commits into
Conversation
|
@anishgirianish somewhat related to what you've been working on recently. WDYT? |
potiuk
added
the
ready for maintainer review
@Dev-iL Thanks for the ping! fix looks good to me, left a couple of small thoughts on the test |
There was a problem hiding this comment.
Pull request overview
Fixes RuntimeTaskInstance.xcom_pull() to respect the caller-provided default value when map_indexes is not set (the NOTSET path), aligning behavior with the explicit map_indexes branch and the method’s documented contract.
Changes:
- Update
xcom_pull()no-map_indexespath to appenddefaultinstead ofNonewhen no XComs are found. - Add a regression test covering the
defaultbehavior whenmap_indexesis not provided andXCom.get_all()returnsNone. - Add a bugfix newsfragment documenting the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| task-sdk/src/airflow/sdk/execution_time/task_runner.py | Fixes xcom_pull() to use default in the no-map_indexes branch when no XCom is found. |
| task-sdk/tests/task_sdk/execution_time/test_task_runner.py | Adds a regression test ensuring default is returned when map_indexes is not specified and no XCom exists. |
| airflow-core/newsfragments/64614.bugfix.rst | Release note entry describing the bugfix. |
|
@nagasrisai — I've removed the Automated triage note drafted by an AI-assisted tool — may get things wrong; a real Apache Airflow maintainer takes the next look once they're resolved. (why automated) Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting |
potiuk
removed
the
ready for maintainer review
|
@nagasrisai — There are 1 unresolved review thread(s) on this PR from @eladkal. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
|
Done — |
…not set On the no-map_indexes code path, both xcom_pull() and axcom_pull() were unconditionally appending None when XCom.get_all() / XCom.aget_all() returned nothing, silently ignoring any caller-supplied default value. Changed both occurrences to append `default` instead of `None`, aligning the no-map_indexes branch with the explicit-map_indexes branch and the documented contract of the method. Also adds three regression tests: - test_xcom_pull_default_respected_when_no_map_indexes - test_xcom_pull_default_is_none_when_not_passed_and_no_xcom - test_xcom_pull_default_fills_correct_position_with_multiple_task_ids
2bcf86b to
6599089
Compare
|
@potiuk - Could you please mark this PR as ready for review? It's currently waiting for a reviewer. Thanks! |
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Thanks. I confirmed the bug is still present on current main: the map_indexes-not-set branch of xcom_pull / axcom_pull appends None instead of default, while the explicit-map_indexes branch and the docstring already honour default. The fix only applies when XCom.get_all finds no rows at all. A stored null XCom comes back as [None] and is still returned as-is, so legitimately-None values are not replaced by default.
Approving, with two small non-blocking nits inline. Please also rebase onto main so we get a fresh CI run before merging — the current run is from August.
@uranusjr @amoghrajesh — since this changes what xcom_pull returns when nothing was pushed (now default instead of None on the no-map_indexes path), could you take a look as well before it's merged?
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
| ) | ||
| xcoms.append(None) if values is None else xcoms.extend(values) | ||
| if values is None: | ||
| xcoms.append(default) |
There was a problem hiding this comment.
This async branch isn't covered: all new tests use the sync xcom_pull. Could you add an async case (e.g. parametrize over sync/async, patching XCom.aget_all)?
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
| ), | ||
| ) | ||
|
|
||
| def test_xcom_pull_default_respected_when_no_map_indexes( |
There was a problem hiding this comment.
This test and test_xcom_pull_default_is_none_when_not_passed_and_no_xcom differ only in input and expected value — could you fold them into one @pytest.mark.parametrize test?
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
| task = CustomOperator(task_id="pull_task") | ||
| runtime_ti = create_runtime_ti(task=task) | ||
|
|
||
| with patch.object(XCom, "get_all") as mock_get_all: |
There was a problem hiding this comment.
Please use autospec=True on the XCom.get_all patches.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Fix
xcom_pull()ignoring thedefaultparameter whenmap_indexesis not set.When
map_indexesis not explicitly provided,xcom_pull()iterates over eachtask_idand callsXCom.get_all(). Ifget_all()returnsNone(meaning no XCom exists for that task), the code was unconditionally appendingNoneto the results list instead of the caller-supplieddefaultvalue. The explicitmap_indexespath did not have this bug.One-line fix:
xcoms.append(None)→xcoms.append(default).Closes #64295
Important
🛠️ Maintainer triage note for @nagasrisai · by
@potiuk· 2026-07-08 15:51 UTCSome review feedback from
@potiukis waiting on you (1 unresolved thread):The ball is in your court — you've been assigned to this PR. Reply or push a fix in each thread, then mark them resolved. See the Pull Request quality criteria.
Automated triage — may be imperfect; a maintainer takes the next look.