-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AIP-72: Port _validate_inlet_outlet_assets_activeness into Task SDK #46020
AIP-72: Port _validate_inlet_outlet_assets_activeness into Task SDK #46020
Conversation
Need to work on few things:
|
Yeah, with the latest changes, the DAG passes: And the error handling is better: CC: @ashb |
Well OK, turns out the test has to be updated now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small comments, LGTM overall though
@ashb handled your comments, let me know if the approval still stands :) |
Yup, still stands. When the changes requested are small enough I often "pre-approve" it to save an extra cycle, and then it's up to you as the PR author to decide it can be merged once the changes are made, or if you need to get a re-review. |
The CI had some issues lately, just working on a green one so that this can be merged |
After a hard battle with the CI, we have won! Merging this one |
closes: #45969
Why?
Tasks can be defined with inlet and outlet events in it definition. For cases when
inactive
assets are defined either in inlets, outlets or both,_validate_inlet_outlet_assets_activeness
is a function defined that checks for this scenario and fails those tasks. This was added as part of #44600.This behaviour needs to be ported into the task sdk to maintain synchronisation with the legacy DAGs written prior to Airflow 3.
Approach
This was done like this earlier:
airflow/airflow/models/taskinstance.py
Lines 3718 to 3729 in 4cab641
AssetUniqueKey
is constructed for aboveairflow/airflow/models/taskinstance.py
Line 3732 in 4cab641
Challenge with doing similar things for task sdk:
inlets
andoutlets
are runtime parameters for tasks, so we can only get them at runtime.So, these lead to two possible approaches:
Approach 1: Performing these checks while starting a
task
ie. in the/run
endpoint.This seemed like the most natural approach but there were a few challenges around here.
This is our start() in supervisor:
We do super().start which starts the child process's entrypoint / target but it doesnt run it until a startup is recveived. And in _on_child_started , we do this:
We tell the server to
self.client.task_instances.start
and then send the startup message to the task runner when it actually starts.The option would be to send the task outlets and inlets (defined as ti.task.inlets or ti.task.outlets) to the execution API. Challenge that makes it hard is that the ti_context comes from the run endpoint and we need that for startup details.
Maybe this can be a later optimisation with fewer network calls but require good amount of refactoring, so parking for now.
Approach 2: Introduce a new endpoint at the execution API that can perform runtime checks and is called after startup and during run from task runner.
/{task_instance_id}/runtime-checks
. Generic endpoint that can perform runtime checks for a TI. Right now limited to_validate_inlet_outlet_assets_activeness
but easily extendable. Returns 200 if all ok, 204 if not applicable and 400 for failed checks.ti.task = ti.task.prepare_for_execution()
through the supervisor and the client.Implementation Details:
Execution API Server
runtime_checks
introduced on task instance, right now limited to checking behaviour assosiated with_validate_inlet_outlet_assets_activeness
but can be extended.{"message": "Runtime checks passed successfully."}
), 204 if not applicable and 400 for failed checks.Client side
Comms
RuntimeCheckOnTask
payload introduced, this one is an extension of payload:OKresponse
:True
indicates that runtime checks passed from server,False
is otherwise.HTTP Client
self.client.post(f"task-instances/{id}/runtime-checks", content=body.model_dump_json())
and translates the response toOKResponse
Task Runner
Supervisor
finish
endpoint with failed state.Models/taskinstance.py
_validate_inlet_outlet_assets_activeness
to a structure that can accomodate the task sdk.The signature now accepts the inlets and outlets directly.
Testing results
DAG defined:
The issue with the DAG is that the same assets are present in the outlets and inlets and changed name with same URI. Hence once of them is inactive in
outlets
Legacy
Logs:
Task SDK
Error logs:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.