Repository navigation
Conversation
dabla
requested review from
amoghrajesh,
ashb,
ephraimbuddy,
jedcunningham and
kaxil
as code owners
July 22, 2026 09:24
amoghrajesh
reviewed
Jul 22, 2026
amoghrajesh
left a comment
Contributor
There was a problem hiding this comment.
I haven't followed up closely with AIP 104, but I have some qns from an initial look.
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
July 22, 2026 14:10
5258e09 to
dbf25d5
Compare
dabla
marked this pull request as draft
August 17, 2026 08:52
Contributor
Author
|
This PR should only be merged once AIP-104 is merged. |
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
September 27, 2026 20:36
a6f73fd to
f688623
Compare
1 task done
Contributor
Author
|
Interaction with #73807 to handle when both land: |
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
12 times, most recently
from
October 1, 2026 10:19
04cc40e to
b74f42c
Compare
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
October 9, 2026 14:18
f2a2cbd to
5594e71
Compare
The success and skip callbacks fired from IndexedTaskRunner.__exit__, as soon as the operator returned and before the indexed task's checkpoint and result were written. A checkpoint write that failed had already announced a success, and the retry ran the indexed task again and announced it a second time; the callback also found no end_date on the task instance. A plain task fires its callback after the push and the state report, with end_date set. The exit now only notes a failure. IterableOperator._run_task calls the runner's report_success() or report_skip() once the matching checkpoint is written, where the indexed task's code ran (its worker thread for a sync operator, the loop for an async one); both set end_date and the state before the callback. A result push that fails after the checkpoint is replayed from it, so the callback fires once. The docs page and the class docstring say so. test_the_success_callback_waits_for_the_checkpoint and the two runner report tests fail before.
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
October 9, 2026 14:48
5594e71 to
f054261
Compare
…valuating the policy again With several failed indexed tasks whose retry policy decisions were all the default, or whose evaluation raised, no exception outweighed the others, the group was handed to the runner and no decision was kept for it. The callbacks then evaluated the policy once more, on the BaseExceptionGroup rather than on any indexed task's exception: K+1 evaluations instead of K, and for a policy that answers differently per call a retry announced here that the runner's own evaluation of the group could refuse. The once-per-item test did not reach this path because its policy answers retry(), so an exception was always chosen. _failure_for_the_runner now keeps the default decision for the group when no decision outweighs it, so _task_will_retry follows it and falls back to retry eligibility without evaluating the policy again. test_an_undecided_group_is_not_evaluated_again_for_the_callbacks fails before with four evaluations.
…the iteration on_kill() reaches the sub-operators that have started, and the stop flag keeps the executor from pulling more. An indexed task instance is neither until its runner starts: on a retry attempt it first reads its checkpoint, and a kill that lands during that read found it unregistered, so it started afterwards, ran to completion unkilled, its remote work included, and the drain waited for it before the task could conclude as terminated. IterableOperator._run_task now asks the iteration state whether a stop was requested once the checkpoint replay is done and before the runner is built, and returns IndexedTaskInstanceNotStarted as the outcome: no code run, no checkpoint, no callback, and the next attempt runs it. IndexedTaskOutcomes counts these apart from the indexed tasks that ran and the kill message names them. A sync indexed task is still handed to the pool afterwards, whose threads are as many as the calls in flight, so only that pickup remains. The docs page and the class docstring list it with the other outcomes that fire no callback. test_an_item_pulled_before_the_kill_but_not_started_does_not_run fails before with two of three indexed tasks run; the outcomes unit test covers the count.
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
October 9, 2026 16:34
f054261 to
0ce65d5
Compare
The static checks fail on D401 for IndexedTaskInstance.context_for: its summary line named what the method returns instead of saying what it does.
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
October 9, 2026 16:51
0ce65d5 to
f482fe4
Compare
The message claimed "the rest never pulled" also when every item had been pulled, and said the input was being resolved for a kill that landed before the run started, where nothing was. IndexedTaskOutcomes._killed_message now names the items that ran, those pulled before the kill and never started and those never pulled, each only when the count is not zero, and a kill before the input was resolved says so. test_a_kill_after_every_item_was_pulled_claims_no_remainder fails before.
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
October 9, 2026 17:35
f482fe4 to
cab90c1
Compare
Contributor
Author
You're right Ash, will update the title to what this PR's functionally adds. |
POST /xcoms/{dag_id}/{run_id}/{task_id}/keys execution API endpoint…puts compare by it
…nt's execution_timeout
… would leave taken
… task instance's identity
…oint XComIterable, the result of an iterated task, stores one XCom per iteration under distinct keys (return_value_0, return_value_1, ...) of the same task instance. The existing slice endpoint for a mapped task's XComs ranges over map_index for a single key, the inverse shape, so iterating or slicing an XComIterable cost one GET per value. The new endpoint takes a list of keys and returns their values in that order, None for a key without an XCom, filtered by map_index (-1 by default), in one database query. Around it: * XComKeysRequest body model, and an AddXComKeysEndpoint execution API version change so older clients keep their contract. * has_xcom_access reads the optional key from the request's path parameters, so the endpoint needs no separate router or dependency. * GetXComByKeys supervisor message, XComOperations.get_by_keys client method, handle_get_xcom_by_keys handler registered with the task supervisor and the DAG processor, an AddGetXComByKeys supervisor schema version entry and the regenerated schema snapshot. * BaseXCom.get_by_keys sends the message and deserializes the values, and XComIterable iterates and slices through it: one request instead of one per value. A single index stays one XCom read. Stacked on the task iteration PR, which owns XComIterable; only this commit belongs to the endpoint. Squashed from the earlier history of this branch and re-based on that PR, with XComIterable's batched reads moved to its current home in bases/xcom.py.
dabla
force-pushed
the
feature/add-xcoms-keys-route
branch
from
October 10, 2026 06:27
cab90c1 to
dcf8439
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #62922 (Task Iteration), which owns
XComIterable: only the last commit, 76625b4, belongs to this PR. Squashed from this branch's earlier history and re-based on that PR, with the batched reads moved toXComIterable's current home inbases/xcom.py.Add a
POST /xcoms/{dag_id}/{run_id}/{task_id}/keysexecution API endpoint thataccepts a list of XCom keys and returns all matching values in a single database
query. Use it in
XComIterableto reduce iteration from N round-trips to one.Motivation
XComIterable(introduced in AIP-104 Task Iteration) stores per-index resultsunder distinct keys (
return_value_0,return_value_1, …) with the samemap_index. The existingGET …/sliceendpoint cannot be reused because itranges over
map_indexfor a single key — the inverse structure. Iterating orslicing a 1000-item result previously issued 1000 separate
XCom.get_onecalls.Changes
POST /xcoms/{dag_id}/{run_id}/{task_id}/keys— accepts{"keys": [...]}, returns values in request-key order (nullfor missingkeys), filtered by
map_indexquery param (default-1)has_xcom_accesssimplified: reads the optionalkeyfromrequest.path_paramsinstead of requiring a{key}path segment, so aseparate router/dependency is no longer needed
XComKeysRequestPydantic model added to execution API data modelsGetXComByKeyscomms message,XComOperations.get_by_keysclient method,handle_get_xcom_by_keyssupervisor handler, and processor dispatch wiringBaseXCom.get_by_keyssends the message and deserializes the values;XComIterable.__iter__and__getitem__slice read through it, oneround-trip for a full iteration or slice. A single index stays one XCom read.
AddXComKeysEndpointexecution API version change andAddGetXComByKeyssupervisor schema version entry, with the regenerated schema snapshot.
TestGetXComByKeys) and forXComIterable(in
test_xcom.py), plus supervisor message coverage.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code following the guidelines
🤖 Generated with Claude Code