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

validate: add separate option for cyclic graph detection #3869

Closed
dpmatthews opened this issue Oct 14, 2020 · 10 comments · Fixed by #4020
Closed

validate: add separate option for cyclic graph detection #3869

dpmatthews opened this issue Oct 14, 2020 · 10 comments · Fixed by #4020
Assignees
Labels
Milestone

Comments

@dpmatthews
Copy link
Contributor

Cyclic graph validation can be quite slow and is currently included as part of --strict.
See cylc/cylc-admin#111 (comment)
See also #3866

@oliver-sanders
Copy link
Member

Note: we could potentially skip cyclic graph detection based on an arbitrary limit to the number of tasks or dependencies.

@MetRonnie MetRonnie self-assigned this Dec 29, 2020
@MetRonnie
Copy link
Member

MetRonnie commented Dec 30, 2020

Something I'm a bit puzzled by: why does the _check_circular() method use [visualization]initial cycle point? Why [visualization]?

def _check_circular(self):
"""Check for circular dependence in graph."""
start_point_string = (
self.cfg['visualization']['initial cycle point'])
lhs2rhss = {} # left hand side to right hand sides
rhs2lhss = {} # right hand side to left hand sides
for lhs, rhs in self.get_graph_raw(
start_point_string,
stop_point_string=None,
):

This also ties into making that section obsolete as part of #3696

@oliver-sanders
Copy link
Member

The visualisation ICP/FCP (Cylc7) define the window that the cylc graph visualisation tool uses (this is a static visualisation tool used to assist with workflow design). So rather than displaying the entire workflow (note the ICP/FCM might not be hardcoded in the workflow anyway) the visualisation settings can define a bit of the workflow that you want to look at.

I guess that graph validation just piggy-backed on this when provided so it ran over a sensible sized chunk of the graph?

Here's the PR that introduced the current system, though the visualisation settings were in use long before: #2332.

@TomekTrzeciak
Copy link
Contributor

How slow is too slow? We do some cycle detection with NetworkX on our suite and even on fairly large graphs this doesn't seem to be a problem. Eg. on a graph with 832 nodes it takes ~1 sec. and for 223987 nodes ~31 secs.

@oliver-sanders
Copy link
Member

How slow is too slow?

<< 1 sec

The intention is to make validate light-weight, so the idea behind the 100-task limit was to run cyclic-graph detection only in trivial cases where we know it will run very quickly.

@TomekTrzeciak
Copy link
Contributor

What happens if I run a suite with more than 100 tasks and circular dependencies? Does it just stall?

@MetRonnie
Copy link
Member

Currently on master, with the following workflow:

[scheduling]
    cycling mode = integer
    [[graph]]
        R1 = a => b => c => d => a

it doesn't run anything:

INFO - Run: (re)start=0 log=1
INFO - Cylc version: 8.0a3.dev
INFO - Run mode: live
INFO - Initial point: 1
INFO - Final point: None
INFO - Suite shutting down - AUTOMATIC
INFO - DONE

@oliver-sanders
Copy link
Member

(same situation as at present, cyclic graph detection is opt-in, currently via --strict)

@MetRonnie
Copy link
Member

There isn't a --strict option for cylc run though?

@MetRonnie
Copy link
Member

After discussing today, this behaviour of cylc run will remain unchanged

@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants