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

TaskState: remove cached prereq satisfaction in preparation for cylc remove #6287

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Aug 6, 2024

These manually cached satisfaction states have the possibility to drift out of sync with individual Prerequisite satisfaction states, especially with cylc remove work in progress #5643 (cylc remove will have to unset any satisfied prerequisites that the removed task provided).

It looks like they are barely used in the first place (at least not anymore):

  • _is_satisfied only has any savings at this call to TaskState.prerequisites_are_not_all_satisfied():

    # Satisfy any absolute triggers.
    if (
    itask.tdef.has_abs_triggers
    and itask.state.prerequisites_are_not_all_satisfied()
    ):
    itask.satisfy_me([

    which is the case for absolute triggers e.g. foo[2001] => bar which are not so common?

  • _suicide_is_satisfied only has any savings at the call above and this call:

    # Event-driven suicide.
    if (
    t.state.suicide_prerequisites and
    t.state.suicide_prerequisites_all_satisfied()
    ):
    suicide.append(t)

    suicide triggers are also not so common.

I am not sure this is strictly necessary for cylc remove (but not happy about the current situation), hence putting it up for review standalone first.

These cached values were first introduced in #1775. There was an issue #1857 mentioning it - looks like it was the cached states drifting out of sync that caused the problem in that case, which is why I am unhappy about it generally.

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).
  • No dependency changes
  • No tests needed
  • No changelog needed as not visible to users
  • No docs needed
  • On master branch as code refactor

This manually cached property has the possibility to drift out of sync with individual `Prerequisite` satisfaction states, especially with `cylc remove` work in progress
@MetRonnie MetRonnie added the code refactor Large code refactors label Aug 6, 2024
@MetRonnie MetRonnie added this to the 8.4.0 milestone Aug 6, 2024
@MetRonnie MetRonnie self-assigned this Aug 6, 2024
@oliver-sanders
Copy link
Member

oliver-sanders commented Aug 7, 2024

Makes sense, we have always assumed that prereqs cannot revert to unsatisfied after being satisfied, however, cylc remove will need to do this.

I expect this cache was introduced for efficiency reasons, may be worth checking that it doesn't perform poorly for tasks with large numbers of prereqs (though I'm not expecting much change by the looks of the code, as you say they are barely used).

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.

Tested with the "Diamond" workflow. CPU usage actually went down slightly as a result of taking out the caching. It definitely wasn't doing anything useful.

@hjoliver hjoliver merged commit b46f35a into cylc:master Aug 8, 2024
27 checks passed
@MetRonnie MetRonnie deleted the remove-prep branch August 8, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code refactor Large code refactors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants