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: detect cyclic without graphviz #2332

Merged

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Jun 29, 2017

Use an algorithm similar to a topological sort to detect cyclic dependence.

cylc validate should no longer require graphviz.

Move remaining graphviz requiring functionality from cylc.config to cylc.graphing.

@matthewrmshin matthewrmshin added the efficiency For notable efficiency improvements label Jun 29, 2017
@matthewrmshin matthewrmshin added this to the next release milestone Jun 29, 2017
@matthewrmshin matthewrmshin self-assigned this Jun 29, 2017
@matthewrmshin
Copy link
Contributor Author

matthewrmshin commented Jun 29, 2017

Selected profile battery results for cylc validate in my environment:

Version Run Elapsed Time (s) CPU Time - Total (s) Max Memory (kb)
7.4.0 complex suite 14.9 15.0 78680
master complex suite 6.2 6.2 70112
HEAD complex suite 4.8 4.7 32280
7.4.0 family-trigger-250 55.7 56.2 439896
master family-trigger-250 22.1 22.7 394164
HEAD family-trigger-250 5.5 5.4 46440
7.4.0 diamond-1000 6.8 6.9 102152
master diamond-1000 4.7 4.7 100408
HEAD diamond-1000 3.4 3.2 66848
7.4.0 nasty-full 1725.85 1723.15 1617328
master nasty-full 173.02 171.68 828168
HEAD nasty-full 163.12 161.68 825564

@hjoliver
Copy link
Member

wow, good work.

@hjoliver
Copy link
Member

As an aside, I wonder if we could use topological sort with weighting by task execution time limit, to estimate suite execution time (e.g. to end of first cycle point). Complicated by clock triggers. https://en.wikipedia.org/wiki/Topological_sorting#Application_to_shortest_path_finding

Copy link
Member

@hjoliver hjoliver 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, tested as working.

[[[P1Y]]]
graph = '''
a[-P1Y] => a
a[+P1Y] => a
Copy link
Member

Choose a reason for hiding this comment

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

Some more ideas for tests.

It would be good to ensure circular dependency identification works between sequences.

[scheduling]
    cycling mode = integer
    initial cycle point = 1
    [[dependencies]]
        [[[2/P3]]]
            graph = foo => bar => baz
        [[[8/P1]]]
           graph = baz => foo

It might also be good to test the "pruning" of dependencies prior to reporting a circular dependency. The example I used yesterday was:

[cylc]
    [[parameters]]
        foo = 1..5
[scheduling]
    [[dependencies]]
        graph = """
            fool<foo-1> fool<foo>
            fool<foo=2> => fool<foo=1>
        """

And for bonus marks to ensure there are no false positives:

[scheduling]
    cycling mode = integer
    initial cycle point = 1
    [[dependencies]]
        [[[1/P3]]]
            graph = foo => bar
        [[[2/P3]]]
           graph = bar => foo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these added.

Use an algorithm similar to a topological sort to detect cyclic
dependence.

Move remaining graphviz requiring functionality from `cylc.config` to
`cylc.graphing`.
Consolidate circular dependence tests in a single test file.
On reporting circular dependence errors, pick out remaining unrelated edges from
the right hand side.
@matthewrmshin matthewrmshin force-pushed the validate-cyclic-without-graphviz branch from 991f5b3 to 4e9972c Compare June 30, 2017 10:24
@matthewrmshin
Copy link
Contributor Author

I have now removed the dependency by cylc list -p on graphviz as well.

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.

A mere order of magnitude difference!

Looks good.

@oliver-sanders oliver-sanders merged commit f3883bf into cylc:master Jun 30, 2017
@matthewrmshin matthewrmshin deleted the validate-cyclic-without-graphviz branch June 30, 2017 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
efficiency For notable efficiency improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants