Repository navigation
Send the Python task's execution_timeout to the supervisor - #75387
jason810496 merged 8 commits into
Conversation
d2facf4 to
cd4f712
Compare
jason810496
left a comment
There was a problem hiding this comment.
I agreed we should make the getter of execution time on both side consistent.
A [core] default_task_execution_timeout set only on the worker no longer applies to Python tasks. It now comes from where the Dag is serialized, same as for language SDK tasks.
Would it be better to still respect the config if we set then fallback to the field set on the TIRunContext?
Since this will introduce breaking change for user IIUC.
We intentionally avoid the value being passed from runner to supervisor in previous PR, but it's fine for the supervisor to respect the config set at the worker.
|
Thanks! Agreed that a worker-only I didn't make the worker config take precedence over Instead, the worker config is used only when the run context has no timeout:
The "equals the worker's default" check matters: the re-parsed value can also come from a Dag file changed since it was serialized, or from |
jason810496
left a comment
There was a problem hiding this comment.
Thanks for tracing this. I agree #74869 has a bug when the Dag processor and the worker resolve execution_timeout differently. It is already on v3-4-test (#75376) but not in any tag yet, so we should fix it before the next 3.4 pre-release.
On v3-4-test, the runner arms SIGALRM from task.execution_timeout, while the supervisor arms from TIRunContext.execution_timeout. When the server value is smaller (a different default_task_execution_timeout, a Dag file changed after serialization, pre_execute, or a mapped expand(execution_timeout=...)), the supervisor sends SIGTERM before the runner raises AirflowTaskTimeout, so on_kill, the retry policy, callbacks and listeners are skipped.
Making the run context the only source for Python tasks fixes that mismatch, but a few cases still end up with no timeout or two different ones:
expand(execution_timeout=...): the server only readspartial_kwargs, so it sendsNone. Values from.map()or from upstream XComs can't be resolved on the server at all.- A core that doesn't send the field: tasks that set
execution_timeoutthemselves get no timeout. The worker-default fallback only covers tasks whose value equals the worker default. BaseAsyncOperatorstill usestask.execution_timeoutinasyncio.wait_for, so it can disagree with SIGALRM.
How about splitting the two paths instead?
- Python tasks:
task.execution_timeoutis the only source. The runner arms SIGALRM with it and sends the same value to the supervisor. - Language SDK tasks: keep using
TIRunContext.execution_timeout, armed by the coordinator as today.
# comms.py
class StartExecutionTimeout(BaseModel):
timeout_seconds: Annotated[float, Field(gt=0)]
type: Literal["StartExecutionTimeout"] = "StartExecutionTimeout"
# supervisor.py
def _handle_start_execution_timeout(self, msg, log, req_id):
self._execution_timeout_seconds = msg.timeout_seconds
self._arm_execution_timeout()
return None, {}
# task_runner.py
SUPERVISOR_COMMS.send(StartExecutionTimeout(timeout_seconds=timeout_seconds))This is close to what #73806 did with SetExecutionTimeout. Both timers then use the same value, and mapped tasks, pre_execute and a worker-only default keep working as before. _resolve_execution_timeout, the supervisor fallback, the create_runtime_ti change and the significant newsfragment can then be dropped, and the TIRunContext.execution_timeout docstring would say it is for tasks run by a language SDK. A language SDK task mapped with expand(execution_timeout=...) would still get None, which we can note as a known limitation.
jason810496
left a comment
There was a problem hiding this comment.
Mark as request change in case of accidental merge.
…uage SDK (#74869)" (#75376) (#75469) This reverts commit 8326025. 3.4.0 closes today and #75376 needs #75387 alongside it, which needs more bandwidth than is left. On v3-4-test alone, the Python task runner arms SIGALRM from task.execution_timeout while the supervisor arms its deadline from TIRunContext.execution_timeout. When the server value is smaller, the supervisor sends SIGTERM before the runner raises AirflowTaskTimeout, so on_kill, the retry policy, callbacks and listeners are skipped. Reverting avoids shipping that regression in 3.4.0. #74869 stays on main. SetExecutionTimeout (#73806) is untouched, so Python tasks keep the supervisor backstop they had before #75376. Co-authored-by: Claude <noreply@anthropic.com>
|
Agree with Jason, and sorry for my incorrect suggestion in #74869. I think we can keep this PR to only "undo" the changes on the Python Task SDK path. Maybe we should also rename One thing we may need to document: with Jason's approach, Python tasks and language SDK tasks read
For language SDK tasks, the default is filled from the Dag processor's config when the Dag is parsed: (WOW it's kinda weird to the users isn't it) |
54b0a67 to
21af315
Compare
|
Thanks both! Reworked along Jason's suggestion, so my earlier reply about the worker-default fallback is outdated.
Two things worth a look:
|
Starting any task deserialized its whole Dag so the route could find out whether the task was a stub and read its arg_bindings. Denormalize the serialized is_stub flag onto task_instance, which ti_run already fetches and locks, and load the Dag only when it is set. execution_timeout is resolved from the same definition, so it is now sent for a lang-SDK task only; apache#75387 has a Python task send its own to the supervisor. Co-Authored-By: Claude <noreply@anthropic.com>
* Load the serialized Dag in ti_run only for stub tasks Starting any task deserialized its whole Dag so the route could find out whether the task was a stub and read its arg_bindings. Denormalize the serialized is_stub flag onto task_instance, which ti_run already fetches and locks, and load the Dag only when it is set. execution_timeout is resolved from the same definition, so it is now sent for a lang-SDK task only; #75387 has a Python task send its own to the supervisor. * Hold SQLite foreign keys off while the downgrade rebuilds Dropping a column rebuilds the table on SQLite, unlike adding one, so the downgrade copies task_instance rows while other tables still point at it. * Pin the bulk-insert path and reword the is_stub comments create_dagrun refreshes every row from its task right after the bulk write, so the existing test passed even when insert_mapping dropped the key. Assert the mapping itself instead, and record what a row keeps when it moves to a new Dag version without being refreshed. * Mark a Go or Java TriggerDagRun task a stub, as TypeScript does * Inline the table name in the is_stub migration ---------
jason810496
left a comment
There was a problem hiding this comment.
After revisit the end to end path, I think the best way is:
- Make this PR revert the loading serialized Dag part and add the
AIRFLOW__CORE__DEFAULT_TASK_EXECUTION_TIMEOUTvia coordinator to pass the env to SDK runtime.
airflow/task-sdk/src/airflow/sdk/coordinators/_subprocess.py
Lines 193 to 206 in b19bc48
- Make each SDK emit the
SetExecutionTimeoutjust like the normal Python task does, so that we can respect the value that user explicitly set, worker env and the default value if unset while we won't introduce the inconsistent behavior as mentioned in #75387 (we shouldn't allow this be happened)
Sorry, I didn't realized we will load serialized Dag for every task on execution API server back then.
Go and TS SDKs built from main now need Airflow 3.5, since a 3.4 supervisor rejects schema 2027-02-28. Is that OK?
Sure, it's fine.
A language SDK task mapped with expand(execution_timeout=...) still gets None. I listed it as a known limitation.
Yes, all the SDKs so far haven't supported the mapped task yet.
jason810496
left a comment
There was a problem hiding this comment.
As discussed offline, the appropriate direction will be having the API server still return the execution_timeout (but only under the is_stub path. We already load the serialized Dag in the is_stub path at API server, there's no harm to return the execution_timeout, the cost is still the same) since we couldn't resolve the execution_timeout defined "StubOperator" at the SDK runtime.
We could resolve the "execution_timeout" in the backend process (#75184) though, but there's more risk of having the subprocess get stuck before returning the execution_timeout to supervisor.
21af315 to
6fd3f10
Compare
|
The first sentence in the description seems backwards? It was tru before #75795, but in current main, a Python task's backstop is never armed, and that regression is the reason to merge this PR. |
Thanks for correction. Updated the description: it now starts with the backstop never being armed for Python tasks on main, and keeps the timer mismatch as the reason the runner sends its own value instead of reading the run context again. |
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the update, LGTM after address the following nits, thanks.
shivaam
left a comment
There was a problem hiding this comment.
LGTM. Tested with ts sdk as well
Only a task a language SDK runs needs its definition from the serialized Dag, so keeping every use of it under that branch makes it clear a Python task never loads one.
A language SDK runtime has to tell a task that sets no timeout, which gets the worker's default, from one that asks for none. Sending zero for the latter needs a negative timeout to stay negative, so the supervisor, not the server, turns it into the grace period alone.
69d2458 to
27a5bca
Compare
Follow-up to #74869, reworked after review.
Since #75795,
ti_runonly loads the serialized task for stub tasks, so a Python task no longer getsTIRunContext.execution_timeout. The supervisor arms its backstop from that value, so onmainthe backstop is never armed for Python tasks. The runner's SIGALRM still works for normal timeouts, but if the task is stuck in native code (or segfaults), nothing kills it.Going back to reading the run context for Python tasks isn't a good fix either. Before #75795 that's what we did, and the supervisor's value could differ from the
task.execution_timeoutthe runner uses (different[core] default_task_execution_timeouton Dag processor and worker, Dag file changed after serialization,pre_execute, mappedexpand(execution_timeout=...)). If the supervisor's was shorter, it sent SIGTERM before the runner raisedAirflowTaskTimeout, andon_kill, retries, callbacks and listeners were skipped.So this PR takes the timeout from wherever the task is actually defined:
task.execution_timeoutfrom the parsed Dag file (same as 3.3) and sends it to the supervisor withSetExecutionTimeout(timeout_seconds). This is the message from Enforce execution_timeout from the task supervisor side #73806 that shipped in 3.4.0, replacingStartExecutionTimeout. Supervisor and runner now use the same value.ti_runstill returnsexecution_timeoutonly foris_stubtasks, as onmain. That path already loads the serialized task forarg_bindings, so no extra cost. The runtime has no way to read the timeout set on a Python@task.stub(execution_timeout on @task.stub tasks is not enforced #74716), so it has to come from the server.TIRunContext.execution_timeoutis now sent as-is, with the same meaning the Python runner givestask.execution_timeout:0The server no longer clamps negative values to
0, since runtimes need0to mean "no timeout" rather than "not set".#74869 was reverted from
v3-4-test(#75469), so 3.4.0 doesn't haveTIRunContext.execution_timeout. The schema changes move from2026-10-30to2027-02-28:AddExecutionTimeoutToTIRunContextmoved tov2027_02_28.py.v2027_02_28.pywithAddExecutionTimeoutToSupervisorTIRunContext, becauseStartupDetailscarries the sameTIRunContextand the runtimes will read the field in the follow-ups.SetExecutionTimeoutis unchanged from 3.4.0, so it needs no entry.mainneed Airflow 3.5.Follow-ups
With this PR, a stub without
execution_timeoutstill gets the Dag processor's[core] default_task_execution_timeout, same asmaintoday. The follow-ups move that default to the worker, like for Python tasks:@task.stubstores noexecution_timeoutwhen the Dag doesn't set one, and0when it's set toNone(default_argscount as set).client_defaultsin the serialized Dag stop carryingexecution_timeout, otherwise the Dag processor's default comes back on read.AIRFLOW__CORE__DEFAULT_TASK_EXECUTION_TIMEOUT. The runtime uses the run context value, falls back to that default, and sendsSetExecutionTimeout.1 drops the default that Go/TS tasks get today and 2/3 bring it back from the worker, so 1 should go in with or after 2 and 3. All three should land in 3.5.0.
Known limitations
expand(execution_timeout=...)get no value. No language SDK supports mapped tasks yet anyway.execution_timeoutyet. After follow-up 1, a Java task without a timeout won't get the Dag processor's default anymore.