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

Remove old graph #32958

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Remove old graph #32958

merged 3 commits into from
Aug 2, 2023

Conversation

bbovenzi
Copy link
Contributor

Remove the old graph view and redirect all requests to the new graph in the grid view.

Part of #29852


^ 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 kind:documentation labels Jul 31, 2023
@bbovenzi bbovenzi added this to the Airflow 2.7.0 milestone Jul 31, 2023
@jedcunningham
Copy link
Member

Overall this looks pretty good. One thing I noticed is the graph tab in the nav doesn't get selected. Are we able to make that work?

Screenshot 2023-08-01 at 4 42 51 PM

@bbovenzi
Copy link
Contributor Author

bbovenzi commented Aug 2, 2023

Overall this looks pretty good. One thing I noticed is the graph tab in the nav doesn't get selected. Are we able to make that work?

Screenshot 2023-08-01 at 4 42 51 PM

Got it working!
change tab

Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

LGTM but better to also be reviewed by @pierrejeambrun

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looking good, glad to see this one removed in favor of our new modern graph details! 🎉

@jedcunningham jedcunningham merged commit 154deed into apache:main Aug 2, 2023
@jedcunningham jedcunningham deleted the remove-old-graph branch August 2, 2023 23:15
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 3, 2023
ephraimbuddy pushed a commit that referenced this pull request Aug 3, 2023

Verified

This commit was signed with the committer’s verified signature.
ephraimbuddy Ephraim Anierobi
(cherry picked from commit 154deed)
@potiuk
Copy link
Member

potiuk commented Dec 5, 2023

This new graph conveys much less information than the past graph and is far more difficult to follow in complex dags. Hovering over no longer isolates the upstream and downstream tasks. All the nodes are the same color, so its hard to pick out specific parts of a large dag. We should have the choice of views in the config.

I suggest you look at latest version and if you have proposals how to improve, open an issue describing it. Or better. PR improving it.

@potiuk
Copy link
Member

potiuk commented Dec 5, 2023

Ideally with screenshots before, after (latest version) and clearly marking what exactly the problem you want to solve.

@potiuk
Copy link
Member

potiuk commented Dec 5, 2023

I think I asked you to open a new issue with those suggestions. It makes no sense to add comments to closed and merged PR. Those are interesting comments and adding them as suggestions for improvement (ideally each suggestion separately) is the only way it wil become actionable.

@wpromatt
Copy link

wpromatt commented Jan 19, 2024

@potiuk The only thing I can't figure out with this new view is how to go to the "graph" view from a link to the task logs. The redirection doesn't work when the dag run you're trying to view in a graph is outside the default selected number of dag runs.

Link to task logs. Note how the "graph" button goes to /dags/ninjacat_listener/graph?root=&execution_date=2024-01-17T21%3A08%3A00%2B00%3A00.

Screenshot 2024-01-18 at 8 09 28 PM

Since this particular dag run was like 200 or so dag_runs in the past, once redirected the graph shows without a selected run. I can only go there by manually adding &num_runs=200 to the url that graph link sends you to.

Is there a way around this? Having that old link also specify the &base_date= param would help a bit. However, in another one of our use-cases we have a DAG that is triggered by a TON of other DAGs. The execution_date for these triggered runs varies only in microseconds, so we can have thousands of dag_runs with execution dates in the same minute or even second. Setting the base_date here wouldn't work. Is there a way to be able to link to the graph view of a DAG without having to set num_runs=2000 in this case?

Maybe I'm missing something obvious.

@wpromatt
Copy link

wpromatt commented Jan 19, 2024

This is also true when clicking on dag_run from the list dag_run tables.

Created an issue.

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 kind:documentation type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants