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

Set execution polling delays correctly #4844

Merged
merged 5 commits into from
May 5, 2022

Conversation

dpmatthews
Copy link
Contributor

@dpmatthews dpmatthews commented Apr 29, 2022

These changes partially address #3706

Tested with this workflow:

[scheduling]
    [[graph]]
        R1 = nolimit & limit95M & limit1H  & limit10M & limit70S
[runtime]
    [[root]]
        script = "echo Hello"
        execution polling intervals = 3*PT30S, PT10M, PT1H
    [[nolimit]]
    [[limit95M]]
        execution time limit = PT95M
    [[limit1H]]
        execution time limit = PT1H
    [[limit10M]]
        execution time limit = PT10M
    [[limit70S]]
        execution time limit = PT70S

Result using master:

$ grep "execution timeout" ~/cylc-run/bug.polling-intervals/runN/log/workflow/log
2022-04-29T15:45:33+01:00 INFO - [1/limit10M running job:01 flows:1] health: execution timeout=PT20M, polling intervals=20*PT30S,PT1M,PT2M,PT7M,...
2022-04-29T15:45:33+01:00 INFO - [1/limit1H running job:01 flows:1] health: execution timeout=PT1H10M, polling intervals=3*PT30S,5*PT10M,PT9M30S,PT2M,PT7M,...
2022-04-29T15:45:33+01:00 INFO - [1/limit70S running job:01 flows:1] health: execution timeout=PT11M10S, polling intervals=2*PT30S,PT1M10S,PT2M,PT7M,...
2022-04-29T15:45:33+01:00 INFO - [1/limit95M running job:01 flows:1] health: execution timeout=PT1H45M, polling intervals=3*PT30S,PT10M,PT1H,PT24M30S,PT2M,PT7M,...
2022-04-29T15:45:33+01:00 INFO - [1/nolimit running job:01 flows:1] health: execution timeout=None, polling intervals=3*PT30S,PT10M,PT1H,...

e.g. limit10M has polling intervals=20*PT30S,PT1M,PT2M,PT7M,... which not what we want.

Result using this branch:

$ grep "execution timeout" ~/cylc-run/bug.polling-intervals/runN/log/workflow/log
2022-04-29T15:43:45+01:00 INFO - [1/limit10M running job:01 flows:1] health: execution timeout=PT20M, polling intervals=3*PT30S,PT9M30S,PT2M,PT7M,...
2022-04-29T15:43:45+01:00 INFO - [1/limit1H running job:01 flows:1] health: execution timeout=PT1H10M, polling intervals=3*PT30S,PT10M,PT49M30S,PT2M,PT7M,...
2022-04-29T15:43:45+01:00 INFO - [1/limit70S running job:01 flows:1] health: execution timeout=PT11M10S, polling intervals=2*PT30S,PT1M10S,PT2M,PT7M,...
2022-04-29T15:43:45+01:00 INFO - [1/limit95M running job:01 flows:1] health: execution timeout=PT1H45M, polling intervals=3*PT30S,PT10M,PT1H,PT24M30S,PT2M,PT7M,...
2022-04-29T15:43:45+01:00 INFO - [1/nolimit running job:01 flows:1] health: execution timeout=None, polling intervals=3*PT30S,PT10M,PT1H,...

e.g. limit10M now has polling intervals=3*PT30S,PT9M30S,PT2M,PT7M,... which makes sense.

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).
  • Already covered by existing tests.
  • Does not need tests (why?).
  • Appropriate change log entry included.
  • No change log entry required (why? e.g. invisible to users).
  • No documentation update required.

@dpmatthews dpmatthews changed the title Set excecution polling delays correctly Set execution polling delays correctly Apr 29, 2022
@dpmatthews dpmatthews marked this pull request as ready for review May 4, 2022 09:15
@oliver-sanders oliver-sanders added this to the cylc-8.0rc4 milestone May 4, 2022
@oliver-sanders oliver-sanders added the bug Something is wrong :( label May 4, 2022
@dpmatthews dpmatthews removed the request for review from datamel May 4, 2022 18:51
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 🎉

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

Final review from me (of change to code base - I have accepted the changes to the test). I have reproduced problem on master to ensure fix works. Merging after CI check.

@oliver-sanders
Copy link
Member

(checks passed)

@oliver-sanders oliver-sanders merged commit 4d19820 into cylc:master May 5, 2022
@dpmatthews dpmatthews modified the milestones: cylc-8.0rc4, cylc-8.0rc3 May 5, 2022
@dpmatthews dpmatthews deleted the polling-intervals branch May 9, 2022 08:17
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.

4 participants