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 clock trigger execution retry delay bug #5286

Merged
merged 5 commits into from
Jan 11, 2023

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Jan 3, 2023

Closes #5217

Fix bug where old-style clock triggered tasks would skip execution retry delays and just retry immediately.

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
  • Tests are included
  • CHANGES.md entry included if this is a change that can affect users
  • No docs needed

@MetRonnie MetRonnie added the bug Something is wrong :( label Jan 3, 2023
@MetRonnie MetRonnie added this to the cylc-8.1.x milestone Jan 3, 2023
@MetRonnie MetRonnie self-assigned this Jan 3, 2023
@MetRonnie MetRonnie modified the milestones: cylc-8.1.x, cylc-8.1.0 Jan 4, 2023
@MetRonnie MetRonnie marked this pull request as ready for review January 4, 2023 16:34
@MetRonnie

This comment was marked as outdated.

Comment on lines -954 to -970
if (
itask.tdef.run_mode + ' mode' in rtconfig and
'disable retries' in rtconfig[itask.tdef.run_mode + ' mode']
):
retry = False
Copy link
Member

Choose a reason for hiding this comment

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

I think this is left behind from an old configuration we removed at Cylc 8?

Copy link
Member

Choose a reason for hiding this comment

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

according to the Cylc 7 code these were obseleted at 7.2.2

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.

I see you've now removed __slots__ declarations from the Interval subclasses. Python docs seems to say this means instances will get an attributes dict as well:

The action of a slots declaration is not limited to the class where it is defined. slots declared in parents are available in child classes. However, child subclasses will get a dict and weakref unless they also define slots (which should only contain names of any additional slots).

I guess that means subclasses with no additional attributes should declare an empty slots tuple?

@oliver-sanders
Copy link
Member

oliver-sanders commented Jan 9, 2023

Could you bump __slots__ fiddling / tidying to another PR to make it easier to get this in for 8.1.0 as the actual fix is only two lines?

@MetRonnie
Copy link
Member Author

Done

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.

LGTM.

Should try to get rid of the sleep in the test as sleeps are a major cause of test flakyness. If the sleep is functional the test could be run in no-detach mode to avoid the issue.

@MetRonnie
Copy link
Member Author

MetRonnie commented Jan 10, 2023

Got rid of the sleep in the test

@MetRonnie
Copy link
Member Author

ZomboMeme 10012023162823

@oliver-sanders
Copy link
Member

They are apparently looking into the upload failure issue.

@oliver-sanders oliver-sanders requested a review from wxtim January 11, 2023 12:46
@wxtim
Copy link
Member

wxtim commented Jan 11, 2023

I'll be happy to merge once the tests have passed.

@MetRonnie MetRonnie removed the request for review from datamel January 11, 2023 14:02
@MetRonnie MetRonnie merged commit 3469a76 into cylc:master Jan 11, 2023
@MetRonnie MetRonnie deleted the clock-trigger branch January 11, 2023 14:03
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.

clock-trigger: incompatibility with execution retry delays
4 participants