dstandish commented on PR #71425: URL: https://github.com/apache/airflow/pull/71425#issuecomment-5258668757
Hi Ephraim. Want to flag a design concern The field `created_dag_version_id` has existed as a column since AIP-65 DAG versioning (#42913/#43735). #49097 added the ORM relationship (created_dag_version) on top of it, with a docstring restating what the name already communicated: "the dag version column that was in effect at dag run creation time." 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. #71455 — only_new clear can report zero new tasks when there genuinely are some, for the same reason. I've filed #71453 to track the underlying contract problem across all of this. 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
