Start porting DAG definition code to the Task SDK #43076
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boring-cyborg
bot
added
area:Scheduler
ashb
force-pushed
the
task-sdk-first-code
branch
from
32a6f3f
to
74dd8f9
Compare
|
This is very much at the "boring ground work stage" - once we've got this covered we can add some of the execution changes which is not of the n"execution interface" side where it gets fun |
jscheffl
reviewed
Oct 16, 2024
There was a problem hiding this comment.
Some comments. In general looks great!
A bit of a challenge to review such a big PR - and I assume it is just the starting :-D - if possible would be cool to split this a bit up for better review, e.g.
- One PR just the skeleton and AbstractOperator or so
- One PR for BaseOperator
- One PR for DAG only
- Maybe one for Nodes/TaskGroup etc clubbing the rest together.
Anyway after some time was able to digest. Would offer also a full review if needed. And please don't take my early comments too serious more or less thinking out loud.
kaxil
reviewed
Oct 16, 2024
|
@kaxil @jscheffl I've just pushed latest changes to this. Probably not worth looking at it again until I've finished it and have a decent idea that the tests are passing locally, but in the last commit I've just pushed I have removed the old code I've ported over, so it might be slightly clearer where I've just moved code. |
ashb
force-pushed
the
task-sdk-first-code
branch
from
2657f36
to
22566d8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ashb
force-pushed
the
task-sdk-first-code
branch
3 times, most recently
from
c0a7b96
to
fd1085d
Compare
kaxil
force-pushed
the
task-sdk-first-code
branch
5 times, most recently
from
29f9cda
to
d4e015a
Compare
ashb
force-pushed
the
task-sdk-first-code
branch
from
d4e015a
to
acdfa2c
Compare
kaxil
force-pushed
the
task-sdk-first-code
branch
from
5bb8796
to
2301523
Compare
ashb
force-pushed
the
task-sdk-first-code
branch
3 times, most recently
from
03282b4
to
11e4304
Compare
This comment was marked as outdated.
This comment was marked as outdated.
kaxil
force-pushed
the
task-sdk-first-code
branch
from
c0d765b
to
3acfb5c
Compare
kaxil
added a commit
to astronomer/airflow
that referenced
this pull request
Oct 29, 2024
While debugging test failures on apache#43076, found that this docstring was wrong, most likely a copy/paste error of `ti._handle_failure`
kaxil
added a commit
that referenced
this pull request
Oct 29, 2024
While debugging test failures on #43076, found that this docstring was wrong, most likely a copy/paste error of `ti._handle_failure`
ashb
force-pushed
the
task-sdk-first-code
branch
from
fc8645e
to
6768541
Compare
…ons in base classes Fix was pydantic/pydantic#8751
Since the scheduler needs it (at least for now), we need to ensure that we include it in our prod-image builds in CI
ashb
force-pushed
the
task-sdk-first-code
branch
from
a94684a
to
1ef4b33
Compare
jedcunningham
approved these changes
Oct 30, 2024
|
There are lots more things to move over to the TaskSDK before AIP-72 will be complete which we will track in the project board, but since this is the big base, and slow to test due to unavoidable wide reaching changes we will merge this and follow up with future changes for the rest (things like Mapped operator, DagParam etc) |
|
Merging this, one of the failures around k8s test that took longer than an hour looks like a transient one: If for some reason it fails on main too, we will debug it |
kaxil
added a commit
to astronomer/airflow
that referenced
this pull request
Oct 30, 2024
Fixing one of the TODOs: apache#43076 This was already addressed.
kaxil
added a commit
that referenced
this pull request
Oct 30, 2024
Fixing one of the TODOs: #43076 This was already addressed.
ellisms
pushed a commit
to ellisms/airflow
that referenced
this pull request
Nov 13, 2024
While debugging test failures on apache#43076, found that this docstring was wrong, most likely a copy/paste error of `ti._handle_failure`
ellisms
pushed a commit
to ellisms/airflow
that referenced
this pull request
Nov 13, 2024
closes apache#43011 By "definition code" we mean anything needed at definition/parse time, leaving anything to do with scheduling time decisions in Airflow's core. Also in this PR I have _attempted_ to keep it to only porting defintiion code for simple DAGs, leaving anything to do with mapped tasks or execution time in core for now, but a few things "leaked" across. And as the goal of this PR is to go from working state to working state some of the code in Task SDK still imports from "core" (various types, enums or helpers) that will need to be resolved before 3.0 release, but it is fine for now. I'm also aware that the class hierarchy with airflow.models.baseoperator.BaseOperator (and to a lesser extend with DAG) in particular is very messy right now, and we will need to think how we want to add on the scheduling-time functions etc, as I'm not yet sold that having Core Airflow depend upon the Task-SDK classes/import the code is the right structure, but we can address that later We will also need to addresses the rendered docs for the Task SDK in a future PR -- the goal is that "anything" exposed on `airflow.sdk` directly is part of the public API, but right now the renedered docs show DAG as `airflow.sdk.definitions.dag.DAG` which is certainly not what we want users to see. Co-authored-by: Kaxil Naik <[email protected]>
ellisms
pushed a commit
to ellisms/airflow
that referenced
this pull request
Nov 13, 2024
Fixing one of the TODOs: apache#43076 This was already addressed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By "definition code" we mean anything needed at definition/parse time, leaving
anything to do with scheduling time decisions in Airflow's core.
Also in this PR I have attempted to keep it to only porting defintiion code
for simple DAGs, leaving anything to do with mapped tasks or execution time in
core for now, but a few things "leaked" across.
And as the goal of this PR is to go from working state to working state some
of the code in Task SDK still imports from "core" (various types, enums or
helpers) that will need to be resolved before 3.0 release, but it is fine for
now.
I'm also aware that the class hierarchy with
airflow.models.baseoperator.BaseOperator (and to a lesser extend with DAG) in
particular is very messy right now, and we will need to think how we want to
add on the scheduling-time functions etc, as I'm not yet sold that having Core
Airflow depend upon the Task-SDK classes/import the code is the right
structure, but we can address that later
We will also need to addresses the rendered docs for the Task SDK in a future
PR -- the goal is that "anything" exposed on
airflow.sdkdirectly is part ofthe public API, but right now the renedered docs show DAG as
airflow.sdk.definitions.dag.DAGwhich is certainly not what we want users tosee.
closes #43011