-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Do not require dag_version_id in workload TI model #55717
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
Do not require dag_version_id in workload TI model #55717
Conversation
o-nikolas
left a comment
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.
Looks like API spec needs some updating, also should we have some tests added or at least updated to test this edgecase?
Older TIs won't have the dag_version_id, so we shouldn't require it in workload's TI model.
c14b250 to
fac4f4a
Compare
Thanks, updated |
| """Schema for TaskInstance with minimal required fields needed for Executors and Task SDK.""" | ||
|
|
||
| id: uuid.UUID | ||
| dag_version_id: uuid.UUID |
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.
How can we execute a TI without a dag version? For it to be executed, doesn't it have to still exist, and this get reparsed to include a dag version?
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.
We can execute a TI without a dag_version since the RuntimeTaskInstance model doesn't include the dag_version_id:
| class RuntimeTaskInstance(TaskInstance): |
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.
Isn't that dag_version_id the one that the (git, etc) dag bundle backend is told to checkout?
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.
Ah no, bundle_info is what the dag bundle info is.
I still wonder how it's actually possible to get to an executor with no dag_version set?
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.
No. dag bundle backend checks out dag bundle version:
| version = Column(String(200), nullable=True) |
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.
And it's more about the dagrun than task instance
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.
Still -- aren't "all" DagRuns in airflow 3 meant to have a version? What is the case by which we end up without a dag version but the task in the executor?
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.
I will experiment with this by reproducing this with a running TI
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.
Alternative to this that's less disruptive: #55884
|
Closing as the alternative has been merged |
Older TIs won't have the dag_version_id, so we shouldn't require it in workload's TI model.
closes: #55713