-
Notifications
You must be signed in to change notification settings - Fork 17.6k
Add team_name to DagRunProtocol to match the DagRun Pydantic model.
#68403
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
Open
dkranchii
wants to merge
2
commits into
apache:main
Choose a base branch
from
dkranchii:sync-dagrunprotocol-team-name
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
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.
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.
This feels like the wrong place to expose this. team_name is not a property of the dag run, but of the DAG.
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.
@ashb You're right that team_name is semantically a property of the Dag (resolved through its bundle), not of the run - every DagRun of the same Dag has the same team_name, and there's no team_name column on the DagRun ORM either.
Where it does already live is the server-side Execution API DagRun payload - it was added there (see AddTeamNameField in execution_api/versions/v2026_06_16.py) so it can be delivered as part of TIRunContext at task start, saving the worker an extra get_dag() round trip. This PR is just keeping DagRunProtocol in sync with that server schema so SDK consumers reading dag_run.team_name don't hit a type-checker failure.
I've pushed a comment on the field that spells this out - the denormalization is a deliberate choice at the server-schema layer, not something we're claiming is the "true" home of the field. If you'd rather see this promoted to DagResponse (and thus DagResult on the SDK side) with team_name removed from DagRun, happy to open a follow-up that does that server-side refactor - but I think it's worth keeping the SDK protocol in sync with the server as long as the server exposes it here.
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.
Yeah, basically this was the cleanest way to plumb the team_name to the context on the worker side.
This isn't even true, I wish it was, but the community would not allow it to be a property of DAG or Task (or indeed DagRun) when designing the multi team feature. It is only a property of the Dag Bundle, everything else (Dag, Dag Run, task, etc) all just infer from Bundle.
dag_run sub model on the TIRunContext already has conf, notes, triggering_user_name, etc and it was the closest thing that maps to team_name. And adding a whole new model just for team_name is way over kill. And there is already code that iterates over dag_run and exports the AIRFLOW_CTX_* env vars.
tl;dr: It was the closest match without adding hundreds of lines of code and new mechanisms just for team, this PR is just updating typing.
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.
@ashb Given the design context @o-nikolas laid out above - team lives on the bundle and DagRun sub-model on TIRunContext is the pragmatic carrier that already ships from the server - this PR is purely a typing sync with the already-shipped Execution API schema. If moving team_name off DagRun server-side is desirable, happy to file a follow-up issue. If you're OK with landing this typing sync as-is, could you resolve the thread?