Repository navigation
Respect run on latest version when clearing a running Dag run - #71425
Conversation
pierrejeambrun
left a comment
There was a problem hiding this comment.
LGTM, but would love another pair of eyes.
Just one nit
|
Hi Ephraim. Want to flag a design concern The field Documented or not, the name itself was always the contract — a write-once historical fact, not a live pointer. Per a conversation with @jedcunningham (the original author of AIP-65), that was intentional: bundle-versioned (pinned) runs were always meant to execute the version pinned at creation, with no path to move off it. "Run with the latest code" was supposed to be achieved by not using a versioned bundle — unpinned bundles already always resolve to the latest DagVersion (DBDagBag._version_from_dag_run). Starting with #54984 and extended through #59764 and #65835/#66901, created_dag_version_id started being mutated after creation to support run_on_latest_version for pinned runs — a capability the original design didn't intend to exist for that case. This PR extends the same mutation to running/queued runs, going further in that direction. That mutation has already produced concrete, reachable bugs elsewhere in the codebase, because other code correctly assumed the field's original (immutable) contract: #71454 — DagRun.dag_versions silently drops versions still in use by task instances that weren't part of a given clear. Given the original design intent, I think this is worth pausing on: should run_on_latest_version apply to bundle-versioned/pinned runs at all, rather than being extended further here? If the answer is yes and this is a deliberate, considered departure from the original pinning guarantee, it should probably written down somewhere (and the field's contract/docs updated to match), rather than continuing to build on a field whose name and documentation still say something the code no longer does. |
Thanks for raising this. I agree that the name and documentation of I don’t think that should block this PR, though. #71425 does not introduce the mutation for queued/running Dag runs: The #71455 repro also does not reproduce on the finished-run path as written. That path calls I agree #71454 is valid. On the broader design question, From the consumers I checked, the execution paths use My preference is therefore to land #71425 to restore the existing invariants, fix #71454 and Drafted-by: Codex (5.6 Sol); reviewed by @ephraimbuddy before posting |
|
Let's rebase and get this in. So what we can then get #73915 merged too |
|
Looking forward to this change, can we backport it to 3.3-x |
dheerajturaga
left a comment
There was a problem hiding this comment.
This looks good. Lets merge this asap to get it in 3.4.0 . I was unable to rebase given I dont have write access to this branch. (need someone from astronomer here)
@bbovenzi , @ephraimbuddy Can you rebase? we should be able to merge
next release is 3.4 . 3.3.x is not planned |
run_on_latest_version should have the same meaning for running and queued Dag runs, and when callers preserve a finished run's state. Otherwise cleared running tasks can retry against stale code while the Dag run still points at an older serialized Dag or bundle. Keeping the run, cleared task instances, integrity checks, and bundle selection aligned preserves the user's explicit rerun choice without rewriting unrelated task-version history.
Co-authored-by: Pierre Jeambrun <pierrejbrun@gmail.com>
4c56b59 to
e61db15
Compare
dheerajturaga
left a comment
There was a problem hiding this comment.
I re-read this after the rebase and the new third commit (e61db152). Moving the version choice off the RESTARTING row and into complete_restart is an improvement: the terminated attempt keeps the version it actually ran, and only the successor follows the run. My earlier approval stands. I left one minor, non-blocking observation inline on taskinstance.py:1244, which can be handled in a follow-up.
Worth a second look from
The third commit also changes how verify_integrity handles mapped tasks. These people know this area best:
@dstandish— wrote 8 of the recent commits tomodels/taskinstance.py/models/dagrun.py, and raised thecreated_dag_version_idcontract question on this PR@uranusjr—CODEOWNERSowner for the mapped-task code (models/mappedoperator.py,models/expandinput.py)
None of them have been notified — asking any of them for an
extra pass is the maintainer's call, and optional.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The findings
below are observations, not blockers; an Apache Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Airflow handles maintainer review:
Contributing guide.
|
On airflow/airflow-core/src/airflow/models/taskinstance.py Lines 1243 to 1244 in e61db15 Minor, and fine to handle in a follow-up PR; it doesn't block this one. When a run has been moved to a newer version, the successor now gets that version's The other task-derived columns don't follow it. The non-running branch of if self.task is not None:
successor.refresh_from_task(self.task, dag_run=self.dag_run)
successor.max_tries = self.try_number + self.task.retriesDrafted-by: Claude Code (Opus 5.5); reviewed by @dheerajturaga before posting |
The rebase onto apache#71425 dropped this PR's clear_task_instances change, and apache#71425 already tests repinning a run after a bundle-only update, so the test no longer exercises anything this PR changes.
The rebase onto apache#71425 dropped this PR's clear_task_instances change, and apache#71425 already tests repinning a run after a bundle-only update, so the test no longer exercises anything this PR changes.
run_on_latest_version should have the same meaning for running and queued Dag runs, and when callers preserve a finished run's state. Otherwise cleared running tasks can retry against stale code while the Dag run still points at an older serialized Dag or bundle.
Keeping the run, cleared task instances, integrity checks, and bundle selection aligned preserves the user's explicit rerun choice without rewriting unrelated task-version history.
Was generative AI tooling used to co-author this PR?
Generated-by: Codex (5.6 Sol) following the guidelines