Conversation
amoghrajesh
left a comment
There was a problem hiding this comment.
This propagates the same coupling I raised in #64751 - isinstance(interval, VariableInterval) here imports airflow.sdk.definitions.deadline.VariableInterval into core airflow at runtime. See #64751 (review) for details.
d906351 to
92c00e3
Compare
|
@amoghrajesh Agreed the counterpart is the real fix, and #71802 is already adding The duplication you flagged is gone too. |
ferruzzi
left a comment
There was a problem hiding this comment.
@amoghrajesh looks like the SDK coupling you flagged has been addressed, and I think this is clear to move now if you agree.
models/taskinstance.py no longer imports from the SDK at all, and the single remaining isinstance(..., VariableInterval) is confined to resolve_deadline_alert_interval() in serialization/decoders.py. Worth noting that decoders.py already imported VariableInterval (function-locally, inside decode_deadline_alert), so this consolidates an existing coupling site rather than adding one, and serialization/definitions/dag.py loses its own import in the process. The core to SDK coupling is one file lower than before the PR.
#71802 then replaces that last isinstance in one place, so nothing here has to move again.
This PR is the only one of the three currently touching _recalculate_dagrun_queued_at_deadlines that is milestoned and backport labelled, so it is the only route this fix has to 3.3.x. More context in my comments on #70714 and #71850.
92c00e3 to
5f2fdd2
Compare
|
@ferruzzi can you review again? |
|
@vatsrahul1001 - Amogh has the blocking review, we'll need him to lift that either way, but yes, a re-review on this one is on my list. |
|
@hkc-8010 - Thanks for your patience on this one. There are a bunch of PRs open which all edit the same portion of code. I've been trying to get them merged in the order that creates the least number of messy merge conflicts, and you are up next (after I get #72651 merged). It's important that you do this merge right since the previous one in the chain added
Passing TLDRHere's my proposal for your fix: Add #71968 added * The technical reason, if you care:This code runs under the the scheduler's |
|
@hkc-8010 72651merged this morning which makes this one the head of the snake with 5 more blocked behind it. I'd love to get this one rebased, re-reviewed, and merged ASAP. @amoghrajesh - I think your 5 August concern is already retired. You flagged the |
…s stored as JSON Since 3.3.0 the deadline_alert.interval column is JSON (migration 0117, apache#64751), holding a serialized timedelta or VariableInterval. _recalculate_dagrun_queued_at_deadlines still passed that raw dict into timedelta(seconds=...), so clearing a DAG run or task instance whose DAG has a DagRunQueuedAtDeadline failed with "TypeError: unsupported type for timedelta seconds component: dict" (HTTP 500). Decode the interval the same way DagRun creation does (decode_deadline_alert), then resolve a VariableInterval to a timedelta before recalculating. The existing test stored the interval as a plain float and never exercised the JSON path; it now stores the serialized envelope and is parametrized over fixed-timedelta and VariableInterval. closes: apache#70368
Move the decode and interval resolution into decode_deadline_alert_model() and resolve_deadline_alert_interval() in serialization/decoders.py, so the deadline recalculation on clear does not need its own copy or an airflow.sdk import. SerializedDAG._process_dagrun_deadline_alerts was doing the same two steps inline and now calls the helpers, dropping its VariableInterval import. closes: apache#70368
5f2fdd2 to
f83bb26
Compare
|
@ferruzzi Rebased onto main with #72651 in. The def resolve_deadline_alert_interval(
alert: SerializedDeadlineAlert, *, session: Session | None = None
) -> datetime.timedelta:and both DagRun creation and the clear path pass theirs. On the test, there are three, and each one goes red if the argument is dropped (I checked by deleting it and re-running):
One other thing the rebase turned up, since you are sequencing these. #72651 made callback decoding strict, and the clear path decodes the callback now where before it only touched the interval. So the raw @amoghrajesh agreed on the coupling, there is nothing left in this path. The SDK import check exits clean and |
Summary
Clearing a DAG run or task instance whose DAG defines a deadline referencing the run's queued-at time (
DeadlineReference.DAGRUN_QUEUED_AT) raisedTypeError: unsupported type for timedelta seconds component: dictand surfaced as an HTTP 500 in the API server.Since 3.3.0 the
deadline_alert.intervalcolumn is JSON (migration0117_3_3_0_change_deadline_interval_to_json.py, from #64751), holding a serializedtimedeltaor variable-backed interval, e.g.{"__classname__": "datetime.timedelta", "__version__": 2, "__data__": 3600.0}._recalculate_dagrun_queued_at_deadlines(called fromclear_task_instances) still passed that raw dict intotimedelta(seconds=...). Every other consumer (DagRun creation inserialization/definitions/dag.py, the model__repr__) decodes the JSON first; only the clear path was missed.DagRun creation already had the decode plus the interval resolution written out inline, so rather than copy it into the clear path, both steps move into
serialization/decoders.pyand both call sites use them.Changes
airflow-core/src/airflow/serialization/decoders.py: adddecode_deadline_alert_model()(decode aDeadlineAlertORM row) andresolve_deadline_alert_interval()(resolve aSerializedVariableIntervalto atimedelta).decode_deadline_alertgets its missing return annotation.airflow-core/src/airflow/models/taskinstance.py:_recalculate_dagrun_queued_at_deadlinescalls the two helpers instead of building the encoded dict itself.SerializedVariableInterval.resolve()raisesValueErrorwhen the Variable is missing or is not an integer, anddecode_deadline_alertraises on a payload it refuses to decode, so on the clear path those are caught and logged and that deadline is left at its old time rather than failing the whole clear. DagRun creation still raises.airflow-core/src/airflow/serialization/definitions/dag.py:_process_dagrun_deadline_alertswas doing the same two steps inline and now calls the helpers, so itsDeadlineAlertFields,SerializedVariableInterval,DATandEncodingimports are gone.Session forwarding
#71968 added the
sessionparameter toSerializedVariableInterval.resolve(), and #72651 started passing it from DagRun creation. That matters: this runs under the scheduler'sprohibit_commitguard, and without an explicit sessionVariable.getfalls through to@provide_session, which hands back the scheduler's own scoped session and then tries to commit and close it.Moving the resolution into a helper means the helper has to carry that through, so
resolve_deadline_alert_interval()takes a keyword-onlysessionand both call sites pass theirs. Three tests cover it, and each one fails if the argument is dropped:test_forwards_session_to_variable_lookup(newtests/unit/serialization/test_decoders.py) on the helper itself.test_dagrun_deadline_variable_interval_uses_callers_sessionon the DagRun creation path, which had no coverage of this before.test_clear_task_instances_recalculates_dagrun_queued_deadlines[variable_interval]assertsVariable.getis called with the same session object the clear is running in, not just a non-Noneone.Tests
airflow-core/tests/unit/models/test_taskinstance.py: the existing test stored the interval as a plain float, so it never exercised the JSON path. It now stores the serialized envelope and is parametrized over a fixedtimedeltaand a variable-backed interval.test_clear_task_instances_skips_deadline_with_unresolvable_intervalis new and covers the skip. Both fixtures also had to start storing a serialized callback, since Decode deadline alert interval and callback without generic deserialization #72651 made callback decoding strict and the old raw{"path": ...}dict is no longer a validcallback_def.mainwith the reportedTypeError.SDK coupling
This PR adds no
airflow.sdkimport to core. #71802 and #72651 have both landed since the last review, soresolve_deadline_alert_interval()isinstance-checks the coreSerializedVariableIntervaland there is no runtime SDK import left in this path.generated/known_sdk_imports_in_core.txtis unchanged by this PR.closes: #70368
PR Checklist
mainbranchprekhooks pass on the changed files, andmypy airflow-coreis clean