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.

Sign up for GitHub

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

Jump to bottom

Task spawning and invalid cycle points #4514

Open
hjoliver opened this issue Nov 16, 2021 · 6 comments
Open

Task spawning and invalid cycle points #4514

hjoliver opened this issue Nov 16, 2021 · 6 comments
Assignees
@hjoliver
Milestone
8.x

Comments

@hjoliver
Copy link
Member

hjoliver commented Nov 16, 2021 •
edited
Loading 

P2 = "foo[-P1] => bar"  # (sequence P2, with erroneous P1 offset)

^ this fails validation in both 7 and 8 with no cycling sequences defined for foo

But this:

P2 = """
   foo
   foo[-P1] => bar
"""

^ at Cylc 7:

  • passes validation (a cycling sequence is defined for foo)
  • foo.1 and bar.1 run (pre-initial ignore)
  • foo runs at every P2 cycle ...
  • ... until suite stalls with prespawned bars waiting on non-existent off-sequence foos

^ at Cylc 8:

  • passes validation
  • foo.1 and bar.1 run (pre-initial ignore)
  • foo runs at every P2 cycle
  • it does not stall, and no bars run

At first glance this is the expected result. Cylc 8 doesn't pre-spawn waiting task so we should not expect a stall here.

But on closer inspection...

At Cylc 7 the graph above is interpreted as :

  • foo is valid at every P2 point
  • bar at every P2 point depends on foo at non-existent off-sequence points

At Cylc 8, the same is true for prerequisite definition, but for spawning purpose (spawn on demand) the second line gets interpreted as:

  • foo => bar[ P1]

So bar doesn't run (at Cylc 8) because foo on P2 points tries to spawn bar at off-sequence points, which is technically not correct.

I don't think that matters in practice, because:

  • it works correctly when there isn't an offset error in the graph
  • the result is the same (albeit for a bass-ackward reason) when there is an offset error: the child bar doesn't run

And bar prerequisites are still correct if they get spawned by another another task:

P2 = """
    foo
    foo[-P1] => bar
    qux => bar
"""

^ This correctly stalls at Cylc 8, with bar on P2 points waiting on non-existent off-sequence foo.

Still, it raises some questions:

  • can we detect these problems at validation?
  • can we detect that foo[-P1] => bar is invalid if foo only exists at P2 points, to avoid attempting to interpret it incorrectly as foo => bar[ P1]?
@hjoliver hjoliver added the question Flag this as a question for the next Cylc project meeting. label Nov 16, 2021
@hjoliver hjoliver added this to the cylc-8.x milestone Nov 16, 2021
@hjoliver hjoliver self-assigned this Nov 16, 2021
@hjoliver
Copy link
Member Author

Related example that doesn't have an error in the graph:

[scheduling]
    initial cycle point = next(T06)
    runahead limit = P1
    [[graph]]
        PT6H = "waz"
        T06 = "waz[-PT6H] => foo"

^ waz runs at every PT6H point, and foo.T06 triggers off of waz.T00 only.

In Cylc 8, for spawning purposes, we try to spawn waz => foo[ PT6H] at every PT6H point which is technically incorrect, but it works because foo is invalid except at T06.

@hjoliver hjoliver mentioned this issue Nov 16, 2021
Closed
12 tasks
@TomekTrzeciak
Copy link
Contributor

I've been aware of this behaviour for some time and always felt a bit uneasy about it. I think this matters, particularly for sequences with exclusions (say, you exclude every New Years day on a P1D sequence only to find out that some dependency has stalled your suite while you're on your Christmas break).

For the suite generating framework I've written for IMPROVER I avoided this problem by taking a different approach. I've made the cycling frequency into a property of the task itself rather than the graph of tasks. Then, for a dependency like foo[P1D] => bar the valid cycles are worked out automatically by shifting the foo sequence by P1D offset and taking intersection with the bar sequence. An empty sequence intersection indicates an impossible dependency and exception is thrown. Generally, I've found this easier to work with and reason about, in particular for large and complex graphs (only one place where the task cycling is defined to worry about).

@hjoliver
Copy link
Member Author

I've made the cycling frequency into a property of the task itself rather than the graph of tasks.

Haha, the earliest versions of Cylc did exactly that, before we had a dependency graph to define prerequisites and cycling.

@hjoliver
Copy link
Member Author

I think this matters,...

Presumably if we can detect and fail this sort of graphing error at validation time, you'd be happy?

@TomekTrzeciak
Copy link
Contributor

I've made the cycling frequency into a property of the task itself rather than the graph of tasks.

Haha, the earliest versions of Cylc did exactly that, before we had a dependency graph to define prerequisites and cycling.

Maybe this should be revisited 😉

I think this matters,...

Presumably if we can detect and fail this sort of graphing error at validation time, you'd be happy?

I don't think it's a major problem in practice, but it would help if the validation could catch it.

@oliver-sanders oliver-sanders mentioned this issue Jun 14, 2022
Closed
7 tasks
@oliver-sanders
Copy link
Member

can we detect these problems at validation?

Yes, for the graph window computed by get_graph_raw (i.e. the same window used by the circular dependency check).

Implemented in #4911

@oliver-sanders oliver-sanders removed the question Flag this as a question for the next Cylc project meeting. label Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hjoliver hjoliver

Labels
None yet
Projects
None yet
Milestone
8.x
Development

Successfully merging a pull request may close this issue.

config: detect un-satisfiable dependencies oliver-sanders/cylc-flow
3 participants
@hjoliver @TomekTrzeciak @oliver-sanders