-
Notifications
You must be signed in to change notification settings - Fork 93
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
Improve stall logic #2126
Improve stall logic #2126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a premature review, but it looks fine to me. You could easily make a test suite that runs to completion (all succeeded) but doesn't shut down because [cylc]disable automatic shutdown = True
Hi @hjoliver, some background to this... For a while now, a user has been receiving suite stalled notification at the end of a normal run of a rose stem suite - i.e. a non-cycling suite with many tasks. I have been unable to reproduce it until recently. I have been unable to reproduce the problem reliably on master, so thanks for the |
Occasionally, the suite can be marked as stalled, even though the task pool only contains succeeded tasks. This change fixes the problem.
a6349ed
to
5b82c60
Compare
(Test added, which seems to fail consistently on master, but not this branch. However, it is a test for a non-existence entry in the suite log, so it is difficult to guarantee correctness in the future.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is difficult to guarantee correctness in the future
I can't think of a better way to write the test.
One irrelevant comment, OK. Will merge after @hjoliver has seen the new test.
@@ -797,15 +797,21 @@ def warn_stop_orphans(self): | |||
|
|||
def pool_is_stalled(self): | |||
"""Return True if no active, queued or clock trigger awaiting tasks""" | |||
can_be_stalled = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable isn't necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, it is actually necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it is!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. The new test works.
Occasionally, the suite can be marked as stalled, even though the task
pool only contains succeeded tasks. This change fixes the problem.