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

Fix manual triggering of runahead-limited parentless tasks. #4640

Merged
merged 3 commits into from
Feb 4, 2022

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Feb 2, 2022

These changes close #4619

Note: this branch is built on top of #4641 (now merged; rebased onto master)

The problem was, we were not auto-spawning the successor of a manually-triggered parentless task unless runahead limit had been recomputed, and we don't recompute the limit after manual triggering (the limit is irrelevant for that). We need to auto-spawn the successor task regardless, but only release it from runhead if the limit has been recomputed (and if the task is below below the limit, of course).

Requirements 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.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required.

@hjoliver hjoliver added the bug Something is wrong :( label Feb 2, 2022
@hjoliver hjoliver added this to the cylc-8.0rc1 milestone Feb 2, 2022
@hjoliver hjoliver self-assigned this Feb 2, 2022
@hjoliver hjoliver changed the title Fix manual triggering of parentless tasks. Fix manual triggering of runahead-limited parentless tasks. Feb 2, 2022
@hjoliver hjoliver force-pushed the fix-4619 branch 2 times, most recently from 235c261 to b2bc678 Compare February 2, 2022 05:32
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.

Tested out manually, fixes the bug.

Test failure is due to changes in #4641 (comment)

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.

This does fix the example reported in #4619, however, fails for a slightly more nuanced example.

The test on this branch has three cycles with a runahead limit of 1:

1  # active
2  # trigger this one
3  # workflow should run onto this task

This example works fine on this branch (and on #4621).

If we take this example and trigger the task in cycle 3 instead we create a bigger headache for the Scheduler as it needs to step over cycles 2 & 3 to spawn cycle 4 as runahead before cycle 2 has even run.

1  # active
2  # runahead
3  # trigger this one
4  # workflow should run onto this task

This example does not work on this branch (Scheduler shuts down), however, does work on #4621.

Reopened the original with a modification of the test from this branch to cover the 4 cycle variant.

[edit] Something is wrong with my testing of #4621 branch apologies:eyes:

@MetRonnie
Copy link
Member

MetRonnie commented Feb 2, 2022

Can confirm it doesn't work for Oliver's modified case:

[scheduling]
    cycling mode = integer
    runahead limit = P1
    [[graph]]
        P1 = foo
[runtime]
    [[foo]]
        script = sleep 20
$ cylc play workflow
# 1/foo spawns and starts running
# 2/foo spawns in runahead
$ cylc trigger workflow//3/foo
# 3/foo spawns and runs
# When 1/foo, 2/foo and 3/foo finish, workflow shuts down, even though no FCP

@hjoliver
Copy link
Member Author

hjoliver commented Feb 2, 2022

This test is broken because:

Oops, sorry I had an un-pushed commit fixing the stupid bash syntax errors (and the UID syntax too!).

The intended if syntax was this:

if (( CYLC_TASK_CYCLE_POINT == 1 )); then ...

@hjoliver
Copy link
Member Author

hjoliver commented Feb 2, 2022

Fixed the new test, checked it passes here and fails on master (and for the right reasons).

@hjoliver
Copy link
Member Author

hjoliver commented Feb 2, 2022

Can confirm it doesn't work for Oliver's modified case:

Interestingly, it works fine if I trigger any foo with cycle point >= 4 (or 2/foo, on this branch), just not 3/foo.

@hjoliver
Copy link
Member Author

hjoliver commented Feb 2, 2022

The new example is technically a different problem, so I'll put up a new PR for that ... #4644

Tests all pass; this one is good to go.

@hjoliver hjoliver requested review from MetRonnie and oliver-sanders and removed request for oliver-sanders February 2, 2022 22:37
@oliver-sanders
Copy link
Member

I think this is good, doing some testing with the other PR merged on top...

@hjoliver hjoliver merged commit ebc13cf into cylc:master Feb 4, 2022
@hjoliver hjoliver deleted the fix-4619 branch February 4, 2022 00:08
@hjoliver
Copy link
Member Author

hjoliver commented Feb 4, 2022

By the way, this branch is equivalent to the approach of #4621 (for triggering runahead tasks only) but this one might have a small efficiency advantage via not recomputing the runahead limit when it's not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Triggering runahead tasks causes workflow to finish early
3 participants