Skip to content
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

Implement new filtering options in graph view #29226

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

yxiao1996
Copy link
Contributor

@yxiao1996 yxiao1996 commented Jan 30, 2023

Description

This change implements new task filtering options in graph view:

  • filtering downstream tasks of a task
  • filtering upstream and downstream tasks of a task

The current Filter Upstream button is replaces by a dropdown button with 3 options:

Screenshot_20230129_032136

Testing

Tested on local airflow environment, run unit tests by breeze testing tests --test-type WWW


^ 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.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jan 30, 2023
@yxiao1996
Copy link
Contributor Author

emm.. static checks failed, I thought I run it before without a problem

yxiaoa@LAPTOP-JAPRTA3V:~/workspace/airflow$ breeze static-checks -t run-mypy --last-commit
Checking pre-commit installed for /home/yxiaoa/.local/pipx/venvs/apache-airflow-breeze/bin/python

Package pre_commit is installed. Good version 2.20.0 (>= 2.0.0)

Good version of Docker: 20.10.21.
Good version of docker-compose: 2.14.0
Good Docker context used: default.
Run mypy for dev.....................................(no files to check)Skipped
Run mypy for core........................................................Passed
Run mypy for providers...............................(no files to check)Skipped
Run mypy for /docs/ folder...........................(no files to check)Skipped

is it not covering the webserver code?

@yxiao1996
Copy link
Contributor Author

OK, so I should actually run breeze static-checks --all-files --show-diff-on-failure --color always, maybe we should update the static check section in breeze guide to reflect this: https://github.com/apache/airflow/blob/main/BREEZE.rst#running-static-checks

@potiuk
Copy link
Member

potiuk commented Jan 31, 2023

OK, so I should actually run breeze static-checks --all-files --show-diff-on-failure --color always, maybe we should update the static check section in breeze guide to reflect this: https://github.com/apache/airflow/blob/main/BREEZE.rst#running-static-checks

This chapter explain examples of different ways you can run the checks - including --all-files or --last-commit or just a single pre-commit id. And even links to the "pre-commit.com" for even more options (you can add --ref-from, --ref-to for example). The --last-commit is just a shorthand for --ref-from HEAD^ --ref-to HEAD in fact.

It's up to you what you run and you need to consciously choose it. The --last-commit only runs it on last commit - if you have multiple of those (because you add fixups) - it will only run the checks relevant to the last commit (i.e. last fixup). If you squash your all fixup (which I heartily recommend) then it will work as expected and run all the relevant changes. This is WAY faster than running --all-files (which ~10 minutes on my 16-processor machine). Asking all users to run --all-files and loose 10 minutes every time is a bad idea.

Also if you follow the instructions and use pre-commit install - you do not have to run static checks at all usually. They will be run for you automatically when you run commit - only for the commit being committed right now. And if you rebase and amend your local commit, this means tha tin vast majority of cases it will take merely seconds or even less than that to run them.

CI runs --all-files just to be sure that there are no unexpected side-effects. And when you se the output you are free for example to only run locally specific pre-commits that failed to save time with --type (the ID of the pre-commits is auto-completeable and when pre-commit fails in CI it prints the ID of it).

So eventually - it's you who are empowered here and decide what's best command to use. That's why we explain you the options that you need to understand consequences of and you should choose what you think is best in the situation.

But if you have idea how to describe it better - without impacting the freedom of choice and educating users like you on what to do, PRs are most welcome. Maybe my answer will give you better understanding that you will be able to explain better for users like you?

@potiuk
Copy link
Member

potiuk commented Jan 31, 2023

BTW. Also in some cases the pre-commit "include/exclude" might miss a thing. Maybe what you experienced is just a missing include/exclude in .pre-commit that would trigger the righ set of check when certain files are modified (this might be the reason why it did not run for you before. I do not know which tests failed for you before but if you could track it down and find out if the specification in .pre-commit-config.yml are correct, that would be awesome (and a good PR to contribute if you fix it).

@yxiao1996
Copy link
Contributor Author

looks like it times out waiting for CI image

   Progress: Image 3.7                     Waiting: #675 ghcr.io/apache/airflow/main/ci/python3.7:627367d641c8eec38e6209a68ae16a7af813bb59.
  ───────────────────────────────────────────────────────────────────────────────────────── Time passed: 01:59:29 ──────────────────────────────────────────────────────────────────────────────────────────
  Error: The operation was canceled.

@r-richmond
Copy link
Contributor

Thanks for tackling this @yxiao1996. Left a tardy UI mockup suggestion for consideration in #28847 (comment).

@bbovenzi bbovenzi added this to the Airflow 2.6.0 milestone Feb 2, 2023
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool!

Although we need to show a user what filters are enabled. Maybe something along these lines?
Screenshot 2023-02-02 at 3 30 51 PM

Also, the button in the modal should be highlighted if that filter is already enabled. Clicking on a highlighted button removes the filter.

We may even need a "Reset" button so users can go back to the full dag

Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we don't need the UI part. By the time this launches I plan to update the graph quite a lot and I'll integrate these filtering options.

@bbovenzi bbovenzi merged commit a8b2de9 into apache:main Feb 3, 2023
@r-richmond
Copy link
Contributor

By the time this launches I plan to update the graph quite a lot and I'll integrate these filtering options.

Well this sounds exciting. Any notes on what is coming?

@bbovenzi
Copy link
Contributor

bbovenzi commented Feb 3, 2023

@r-richmond
Screenshot 2023-01-27 at 3 23 09 PM

@funyeah
Copy link

funyeah commented Feb 8, 2023

@r-richmond Screenshot 2023-01-27 at 3 23 09 PM

Looks amazing!

Is it possible to add default limit of dependency depth and operators number for graph view?
Currently, graph view loads very slowly when more than 1000 operators, and hardly works when more than 3000.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:new-feature Changelog: New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants