-
Notifications
You must be signed in to change notification settings - Fork 16.4k
feat: add task upstream/downstream filter to Graph & Grid #57237
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
feat: add task upstream/downstream filter to Graph & Grid #57237
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
08c1fd5 to
5e00124
Compare
jscheffl
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 adding the feature!
In order not to add complexity to the layout with another dropdown, could you add the lineage filter to the existing "Options" dropdown - that would be a bit more consistent for the UX in my view?
|
Thanks for your swift reply!
I considered it. The reason I didn't was because a user would not be able to see whether a DAG is filtered or not on first glance. With this extra dropdown there is an instant feedback showing back to the user. Curious to hear your thoughts on that approach. That being said, more than happy to move it to options. For us the feature is the more important thing. I have an additional question: I realized that I have only implemented it for the graph view, and deliberately left out the task overview. However, in Airflow 2 it propagated to both. Shall I try to implement it for both in this PR as well? |
Then before you rework and there might be different opinions, I think it is fair to also wait a second for other UI mainteainers feedback for UX - @pierrejeambrun / @bbovenzi What is your opinion about the UX? |
e5ed9dc to
da02baa
Compare
|
Perfect, in the meantime I took the liberty of ensuring the behaviour is consistent between the Grid and the Graph 👍 . This also simplified the code a bit. Looking forward to hearing from the other UX maintainers! |
f8a51f9 to
88e9bfe
Compare
|
Hey Oscar thanks for tackling this p.s. #53458 was submitted for exactly this. |
Ah I totally missed that one, thanks for the heads up! Regarding the other PR, let me know how you prefer to go ahead. Happy to continue with this PR, I have some time available for it. |
0f55a58 to
4dd842e
Compare
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.
Super cool, I like it a lot.
I'm fine with having that as a separate dropdown than 'settings'.
I would have to play with it more but I think it's going in the right direction
airflow-core/src/airflow/ui/src/layouts/Details/Graph/Graph.tsx
Outdated
Show resolved
Hide resolved
|
airflow-core/src/airflow/ui/src/layouts/Details/Graph/Graph.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/LineageFilter.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/LineageFilter.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/LineageFilter.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/LineageFilter.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/LineageFilter.tsx
Outdated
Show resolved
Hide resolved
|
Thanks all for the feedback. I will try to find some time today or tomorrow to implement and get back to you 👍 |
|
@bbovenzi I tried incorporating the feedback points you left. Thanks for the detailed review, points made a lot of sense to me. Please keep them coming! @ashb, @bbovenzi , I changed the name to TaskStreamFilter. I wanted to be a bit more specific than just TaskFilter, as I think there are more variables that people can use for filtering tasks. However, I'm still not entirely sold by the name. So very open to suggestions. |
f819f6a to
9b7cfa8
Compare
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.
Nice work.
I think a lot of this PR can still be cleaned up.
We should make this filter the same as any table filter in Airflow and make use of searchParams. ?root=task_id&upstream=true. Then we can avoid all of this prop drilling and state management and any component that needs to be aware of the root can just check the searchParams.
a206c78 to
eee5cd2
Compare
That is actually an amazing piece of feedback! I should've thought of that before 😅 . Implemented! Cleans up the hook and a lot of this state juggling. |
e13a5b9 to
8278d6c
Compare
8278d6c to
78c31cf
Compare
|
Just have some linting issue. Please run |
0891fc9 to
c23cac1
Compare
3a3ce3a to
c96adfc
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* feat: add DAG graph lineage filter * feat: propagate task filter to grid and graph view * fix: rename to task stream filter and small improvements * fix: bug in selecting root task in grid view * refactor: use searchParams instead of LocalStorage hook * fix: lint issues and improve light mode visuals
* feat: add DAG graph lineage filter * feat: propagate task filter to grid and graph view * fix: rename to task stream filter and small improvements * fix: bug in selecting root task in grid view * refactor: use searchParams instead of LocalStorage hook * fix: lint issues and improve light mode visuals
* feat: add DAG graph lineage filter * feat: propagate task filter to grid and graph view * fix: rename to task stream filter and small improvements * fix: bug in selecting root task in grid view * refactor: use searchParams instead of LocalStorage hook * fix: lint issues and improve light mode visuals


Overview
We noticed that the lineage filter was missing in the Graph layout. At our company, the DAGs get quite complex, and this was a beloved feature by our users. In this PR, I try to have a crack at implementing it. I did my best to ensure that it feels similar to the old button in Airflow 2 in terms of its behaviour.
UI addition
See image:
When dropping down, a user can select the same options as before:
When selected, it shows at the top what filter is active:
From here, the user can navigate with the active filter. When reselecting this filter dropdown on another task focus, the user gets the ability to take this selected task as root task for the filter:
Please feel free to mess around with it!
Some notes from me on UX:
Note to reviewer
I did some React back in the day, but it's a bit rusty. I'm here to contribute, but definitely also to learn. I'm also Dutch, so please feel free to tell me in a very direct manner which parts make no sense at all 😂 .
Related Issues
I believe this PR can contribute to resolving the below:
closes: Reimplement #29226 for Airflow 3 UI | Filter tasks to Upstream and/or Downstream #53458
But there are more functionalities listed there, that I left out of scope for this PR.
Testing
I am unsure what the procedure is for testing in the UI. I spun up Breeze (love it btw, amazing tool) and clicked around there. Please advice on any further testing procedures or reading materials.
^ 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.