-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Feat: Add version change indicators for Dag and bundle versions in Grid view #53216
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
base: main
Are you sure you want to change the base?
Conversation
09ecc2b to
bd021e8
Compare
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.
Thanks for the PR! I think this is bringing back the deprecated and removed monolith endpoint with its methods. I think we should integrate the fix into only new structure
cc: @dstandish @pierrejeambrun
|
@bugraoz93 Thanks for your quick feedback! I saw something strange while fixing the conflict, I think my endpoints follow the old style. I’ll change them to use the new structure. |
Amazing, thanks a lot! |
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py
Outdated
Show resolved
Hide resolved
|
I’ve updated the tests and removed the old structure, |
|
I’ve tested the scenarios I had in mind, and everything looks good so far 🙌 |
bugraoz93
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.
Thanks for the changes! I believe this PR still contains old deprecated code :)
airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
jason810496
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.
Nice! Thanks for the PR!
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
|
I thought I deleted the old version, but it still there. |
|
If we want to visualize version change, in my opinion, we should do it for bundle version and not dag version that changes everytime especially for dynamic dags cc @jedcunningham |
In that case, we might need to use an icon and then show the bundle version in a tooltip. Maybe something like |
Maybe we need both with different colors, dag version change (to task level) + bundle version change (dag run level only). If we were to keep only one, I would be in favor of keeping the DagVersion because it is consistant with the Graph(Graph is showing structures of specific dag versions), And because I think source code is less important than what was actually executed. |
pierrejeambrun
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.
I think the code looks very complicated for what we are trying to achieve. (And also there are some unused stuff I believe from the legacy grid)
The backend need to serialize the dag_run.versions in the GridRunsResponse, same for tasks.
And in the UI, we just need a simple logic to check if dag_run.version of Nth run is equal to dag_run.version of (N-1)th run.
(Same logic goes for tasks, and yes if DagRun.dag_versions.lenght === 1 no need to go try the task level logic)
Maybe I missed something though.
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx
Outdated
Show resolved
Hide resolved
I'm afraid that we can potentially have as many tooltips as TI in the grid. Which would most likely bring back tooltip performance issue we observed in the past. |
The reason I haven’t resolved the comments yet is that the actions were failing during the freeze period. |
pierrejeambrun
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.
I see some activity on the PR, let us know when this is ready for a second round of reviews. (need rebase and a few comments are still worked on it appears).
Thank you for your work
|
Thanks for taking this on! We should make sure that the display works when the version count is is in the double or even triple digits. Perhaps we should fallback to just use an icon? |
|
I think the git line+circle icon you had before is better. We use the CwRefresh for clearing dag runs Also, we probably will want to add a checkbox in Options to turn off the version indicators. |
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.
Yes that dropdown seems nice to me to be able to hide / display version markers.
Do you have an updated screenshot of what it looks like now with both bundle change and version change on the same grid. (most likely what happens if at the same spot we have both a version change and bundle change)
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
I’ll make sure to update all the feature-related screenshots by tomorrow ;) |
|
@bbovenzi @pierrejeambrun I’ve updated all the screenshots in the description ;) |
bbovenzi
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.
I think we should add a legend to make it easier for users to learn dag versions vs bundle version changes.
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
| transition="all 0.2s" | ||
| width={2} | ||
| /> | ||
| <Box |
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.
Why did we make a custom tooltip instead of the one from chakra? I know there were performance issues with it before, but now it seems much better.
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 used a custom tooltip to prevent it from covering other task status boxes in the Task Instance grid view, allowing it to naturally overlap with the indicator.
However, this approach made the CSS more complex and harder to maintain, so I’ve been reconsidering it.
As you suggested, I think it would be better to use the Chakra tooltip provided in the component for better consistency and simplicity, and I’ll update it accordingly.
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx
Outdated
Show resolved
Hide resolved
|
@bbovenzi I’ve been thinking about the legend, and currently, the horizontal indicator for Dag version changes and the indicator for bundle version changes use the same icon. Here’s an example of what it looks like. What do you think?
|
@bbovenzi @pierrejeambrun This part hasn’t been implemented yet. I’d like to hear your ideas regarding the legend. |
pierrejeambrun
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.
Nice, looking better and better.
A few suggestions to try to make things simpler and improve UX.
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts
Outdated
Show resolved
Hide resolved
| run_after: datetime | ||
| state: DagRunState | None | ||
| run_type: DagRunType | ||
| dag_version_number: int | None = None |
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 don't think you need to pass this down, because the grid is calling get_dag_details and the latest dag_version is already there in the payload.
So you might get it in the front-end from there. same for the bundle name and bundle version.
| run_type: DagRunType | ||
| dag_version_number: int | None = None | ||
| bundle_version: str | None = None | ||
| has_mixed_versions: bool = False |
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.
Same for this. You don't need to compute it. We have the get_dag_run query in the context of the grid, that returns dag_versions already, if the list is bigger than 1 we have mixed version. (only thing is that this list consider TaskInstanceHistory too, which might or might not be appropriate depending on how we want to visualize things)
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.
Removing that will also simplify the backend code.
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 understand the suggestion to reuse the existing APIs, and I explored the frontend-only approach, However, I have some concerns.
One main issue is the Grid shows multiple Dag runs at the same time, and if we try to fetch the version information for each run individually using get_dag_run, the number of API calls quickly adds up.
For example, if there are 10 runs displayed, we’d end up making 11 API calls in total—one for get_dag_details and one for each of the 10 runs.
Could you let me know if what I found out is incorrect?
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
pierrejeambrun
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.
Feel free to mark addressed comments as resolved.
Let me know when this needs another round of review.
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts
Outdated
Show resolved
Hide resolved
|
Except for this part #53216 (comment) |
|
We'll need to rebase this after virtualizing the grid's vertical scroll. |
fad0cf6 to
f48e7ec
Compare





Summary
Adds visual indicators in the Grid view to help users identify Dag version changes and detect mixed-version TaskInstances within a single DagRun.
Problem
Solution
This PR introduces visual cues in the Grid view to address these pain points:
Change
Backend
Frontend
Screenshots
Version Change
LocalDagBundle
Dag Version Indicator Tooltip(vertical)

Dag Version Indicator Tooltip(horizontal)

GitDagBundle
Dag Version Indicator Tooltip

Bundle Version Indicator Tooltip

Dark Mode
Toggle Option(with Legend)
Users can choose 'Show All', 'Show Dag Version', 'Show Bundle Version', 'Hide All'
Show All
Show Bundle Version
Show Dag Version
Hide All
Indicator Scenarios
Scenario 1: Normal
Scenario 2: Version Change
Scenario 3: Mixed Version
Scenario 4: Bundle Version & DagVersion Change
Scenario 5: Only Bundle Change (Dynamic Dag)
closes: #52286
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.