Skip to content

Remove old graph#32958

Merged
jedcunningham merged 3 commits into
apache:mainfrom
astronomer:remove-old-graph
Aug 2, 2023
Merged

Remove old graph#32958
jedcunningham merged 3 commits into
apache:mainfrom
astronomer:remove-old-graph

Conversation

@bbovenzi

Copy link
Copy Markdown
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
Copy Markdown
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

bbovenzi commented Aug 2, 2023

Copy link
Copy Markdown
Contributor Author

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

@eladkal eladkal left a comment

Copy link
Copy Markdown
Contributor

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

@hussein-awala hussein-awala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good!

@pierrejeambrun pierrejeambrun left a comment

Copy link
Copy Markdown
Member

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
(cherry picked from commit 154deed)
@potiuk

potiuk commented Dec 5, 2023

Copy link
Copy Markdown
Member

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

potiuk commented Dec 5, 2023

Copy link
Copy Markdown
Member

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

@potiuk

potiuk commented Dec 5, 2023

Copy link
Copy Markdown
Member

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

wpromatt commented Jan 19, 2024

Copy link
Copy Markdown

@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

wpromatt commented Jan 19, 2024

Copy link
Copy Markdown

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.

8 participants