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

Move max active cycle points behaviour into runahead limit #3848

Merged
merged 10 commits into from
Oct 13, 2020

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Oct 1, 2020

These changes partially address #3667

The functionality of setting a runahead limit of a certain number of consecutive cycle points is being moved from [scheduling]max active cycle points to [scheduling]runahead limit.

In future, max active cycle points will set a limit on non-consecutive cycle points (see #3667 (comment) for info on the difference between consecutive and non-consecutive limits), but this is not implemented in this PR. Edit: max active cycle points is being deprecated and a new setting will set the non-consecutive limit.

Examples:

before after
max active cycle points = 3 runahead limit = P3
runahead limit = PT12H (same as before)

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

@MetRonnie MetRonnie added this to the cylc-8.0a3 milestone Oct 1, 2020
@MetRonnie MetRonnie self-assigned this Oct 1, 2020
@MetRonnie MetRonnie linked an issue Oct 1, 2020 that may be closed by this pull request
@MetRonnie MetRonnie marked this pull request as draft October 1, 2020 12:39
({'cycling mode': 'integer', 'runahead limit': 'PT12H'}, False),
({'cycling mode': 'integer', 'runahead limit': 'P7D'}, False),
({'cycling mode': 'integer', 'runahead limit': '4'}, False),
({'cycling mode': 'gregorian', 'runahead limit': ''}, False),
Copy link
Member Author

Choose a reason for hiding this comment

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

It is not possible to set runahead limit to null

Copy link
Member

Choose a reason for hiding this comment

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

Sensible, an infinite runahead limit is a whole other kettle of fish!

Comment on lines 359 to 372
This limit on the number of consecutive cycle points is specified
by an interval between the least and most recent: either an integer
interval (e.g. ``P3`` - works for both :term:`integer cycling` and
:term:`datetime cycling`), or a time interval (e.g. ``PT12H`` -
only works for datetime cycling). Alternatively, if a raw number is
given, it will be taken to mean that number of hours, though this
usage is deprecated.
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this format of Pn (e.g. P3) for a number limit is good, because if you have

[[graph]]
    P2 = foo  # where foo fails

That means the cycle points will be foo.1, foo.3, foo.5 etc. But the runahead limit will stall at foo.5 not foo.3 because foo.5 is actually the 3rd cycle point.

So runahead limit = 3 instead of P3 would make more sense. But the trouble is it would break the backwards compatibility where the raw number is interpreted as PT3H

Copy link
Member

@oliver-sanders oliver-sanders Oct 1, 2020

Choose a reason for hiding this comment

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

It wouldn't break backwards compatibility because this issue only occurs in integer cycling mode. It might make it a bit harder to parse the value though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting interpreting a raw number as number of cycle points for integer mode, and number of hours for datetime mode? Seems messy to me 😬

Copy link
Member

@oliver-sanders oliver-sanders Oct 1, 2020

Choose a reason for hiding this comment

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

The alternatives would be:

  • Stick with P<n>, just document the hell out of it.
  • Come up with another prefix character (confusing).

Copy link
Member Author

Choose a reason for hiding this comment

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

Stick with P<n>, just document the hell out of it.

That seems the best option to me, if we can't afford to lose the raw hours back compat.

Copy link
Member

Choose a reason for hiding this comment

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

Sadly the old feature was never actually deprecated and is widely used.

Comment on lines -16 to +19
T04 = "run_ok & fail"
T04 = "run_ok"
# T05...
T05 = "run_ok_2 & fail"
T05 = "run_ok_2"
Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't matter whether these succeed or fail, as foo.T00 has already failed, so that is what limits the runahead

- runahead limit can now be used for number of consecutive cycle pts (or
- the pre-existing behaviour - time interval between oldest and newest
cycle pts)
- max active cyle points is currently disabled until a non-consecutive
implementation is ready
Because moved `max active cycle pt` behaviour to `runahead limit`

- Changed default from 3 to 5
- Fixed tests where it was looking up (from the db) the max cycle point
in the runahead pool instead of just the task pool
… limiting

Also change it to an integer cycling workflow as all the other runahead 
tests already test datetime cycling
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.

Few tiny comments.

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
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.

All good, thanks @MetRonnie 👍

@hjoliver
Copy link
Member

(pycodestyle failed; I'll fix that in my add-a-comment follow-up).

@hjoliver hjoliver merged commit ec3a7be into cylc:master Oct 13, 2020
@MetRonnie MetRonnie deleted the runahead-limit branch October 14, 2020 09:31
@MetRonnie MetRonnie added the config change Involves a change to global or workflow config label Nov 13, 2020
@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
config change Involves a change to global or workflow config
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runahead limit / max active cycle points
3 participants