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

added cyclepoint grouping with subgraphs #1763

Merged
merged 6 commits into from
May 17, 2024

Conversation

markgrahamdawson
Copy link
Contributor

@markgrahamdawson markgrahamdawson commented Apr 19, 2024

Partly addresses #1130

  • Added button to graph view to group nodes by cycle point.
  • Works for timeseries and integer
  • Option state saves when moving between workflows

---initial notes on work---
For the time being - figure out a means of drawing a dotted box around nodes that share a cyclepoint.
This can be done with graphviz subgraphs
The graphviz dot code is returned from the getDotCode function in Graph.vue view
Dot code can be rendered with
dot foo.dot -Tpng -o foo.png
ultimately in the svg there will need to be a rect tag added...

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@markgrahamdawson markgrahamdawson self-assigned this Apr 19, 2024
@markgrahamdawson
Copy link
Contributor Author

markgrahamdawson commented Apr 22, 2024

The amount of margin between subgraphs is random unless made explicit

No margin
image

Margin set in code
image

Im guessing the second one is better?

@hjoliver
Copy link
Member

Def prefer the second one.

@MetRonnie MetRonnie added this to the 2.6.0 milestone May 8, 2024
Copy link
Member

@MetRonnie MetRonnie 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, partial review so far

src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
Copy link
Member

@oliver-sanders oliver-sanders 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, works nicely 👍

Sadly, GraphViz doesn't stop edges from being routed through graph labels by the looks of it. Not much we can do about that, but we could try adding a white stroke (SVG lingo for border) around the text to keep it readable, e.g:

element {
    fill: black;
    stroke-width: 2;
    stroke: white;
}

Could do with bumping the font-size of the graph labels a bit, not much, they don't need to be larger than task GraphNodes, but they do get lost in big graphs rather easily e.g:

Screenshot from 2024-05-08 17-14-24

src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/components/cylc/GraphSubgraph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/components/cylc/GraphSubgraph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Show resolved Hide resolved
src/components/cylc/GraphSubgraph.vue Outdated Show resolved Hide resolved
src/components/cylc/GraphSubgraph.vue Outdated Show resolved Hide resolved
src/components/cylc/GraphSubgraph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
added toggle button and styling

added e2e test

use initialOptions prop to save view state

fix nodes undefined error

added towncrier entry

review amends

review amends

fix broken e2e tests
src/components/cylc/GraphSubgraph.vue Outdated Show resolved Hide resolved
src/components/cylc/GraphSubgraph.vue Outdated Show resolved Hide resolved
@oliver-sanders oliver-sanders modified the milestones: 2.6.0, 2.5.0 May 14, 2024
@oliver-sanders
Copy link
Member

Bumping this forward to the 8.3 meta-release as this should be ready to go in before cylc-flow blockers.

It would be good to get a screenshot of this into the "changes" section in cylc-doc: https://cylc.github.io/cylc-doc/nightly_8.3/html/reference/changes.html#cylc-8-3

@oliver-sanders oliver-sanders linked an issue May 14, 2024 that may be closed by this pull request
@MetRonnie
Copy link
Member

Got a PR against this at markgrahamdawson#5

@MetRonnie
Copy link
Member

MetRonnie commented May 14, 2024

Managed to get a workflow into a state where there's an grouping with the title "undefined".

And transposing this graph gives a console error (resolved)

image

@markgrahamdawson
Copy link
Contributor Author

to a state where there's an grouping with the title "undefined". And transposing this graph gives a console error. Will see if I can figure out anything else tomorrow

Do you have the flow.cylc file?

@markgrahamdawson

This comment was marked as resolved.

@oliver-sanders
Copy link
Member

oliver-sanders commented May 15, 2024

Hmm, I didn't notice any issues with the complex workflow when I tested it. Will need the dot code to debug this. Given that the edges (yes that's right word) lead outside of the subgraph, I expect there may be an issue with the formatting of the subgraph dot code.

When I read through the code I think I spotted a missing newline at the end of the subgraph. I thought it was harmless, but it might be worth checking that's not causing issues.

@MetRonnie
Copy link
Member

Managed to get a workflow into a state where there's an grouping with the title "undefined"

Also the 3 tasks are showing as "state unknown" while the jobs are showing as running. But these are from an earlier cycle and have actually already succeeded. Might be something to do with simulation mode. Or actually #1635?

@MetRonnie
Copy link
Member

MetRonnie commented May 15, 2024

Got a fix for the arrows pointing nowhere (and the console error) in markgrahamdawson#5

MetRonnie and others added 2 commits May 15, 2024 15:27
* Tidy

* Mock data: update subscription data for graph view

* Graph view: handle negative coords when converting graphviz edge to SVG
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Regards that "undefined" error for tasks with the missing cyclePoint field.

There is a slightly nicer way to fetch the cycle point that is available for all tasks by default (no need to request the cyclePoint field explicitly) using the "tokens" which are provided for every object in the store. Due to the way tokens are provisioned, the cycle cannot be undefined by accident (it's derived from the ID).

I haven't tested with these changes so worth giving them a spin before committing to them, but hopefully that'll resolve the error.

src/services/mock/json/workflows/multi.json Outdated Show resolved Hide resolved
src/services/mock/json/workflows/multi.json Outdated Show resolved Hide resolved
src/views/Graph.vue Outdated Show resolved Hide resolved
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

(re-approving)

@oliver-sanders
Copy link
Member

👍

@oliver-sanders oliver-sanders merged commit 4ef10a8 into cylc:master May 17, 2024
8 checks passed
@oliver-sanders
Copy link
Member

(added a changes entry in cylc-doc cylc/cylc-doc#733)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph view: group tasks by cycle point
4 participants