-
Notifications
You must be signed in to change notification settings - Fork 94
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 spawning of mixed-parentage tasks. #4906
Conversation
83decfb
to
fd38b0a
Compare
@@ -820,7 +820,8 @@ def select_task_pool_for_restart(self, callback): | |||
LEFT OUTER JOIN | |||
%(task_outputs)s | |||
ON %(task_pool)s.cycle == %(task_outputs)s.cycle AND | |||
%(task_pool)s.name == %(task_outputs)s.name | |||
%(task_pool)s.name == %(task_outputs)s.name AND | |||
%(task_pool)s.flow_nums == %(task_outputs)s.flow_nums |
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.
Bug fix! Loading the task pool from the DB at restart, on master I get multiple rows per task proxy if a waiting task had also run in previous flows (and hence attempt to spawn multiple copies of the same task).
938e4d9
to
4c98f6a
Compare
|
e41a656
to
5183cdd
Compare
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.
Reviewed, and found working (could reproduce the issue and fix) 👍
I reviewed the code, followed most of it...
|
@hjoliver What font are you using in those terminal screenshots? |
Ubuntu Mono Derivative. You like or no like? |
@MetRonnie - this is good to go now. |
I'm happy to review but will be fairly busy this upcoming week with workshops, so someone might like to beat me to it |
That's fine (I just pinged you as you'd self-assigned as a reviewer above). |
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.
Partial review
(Those that have parents in some cycles but not others). Also tidy up task spawning and runahead computation.
Adjust tests accordingly.
Co-authored-by: Ronnie Dutta <[email protected]>
c9e276c
to
965d356
Compare
Me likey, is very nice for Tui. |
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.
Halfway through...
Co-authored-by: Ronnie Dutta <[email protected]>
Co-authored-by: Ronnie Dutta <[email protected]>
[[events]] | ||
inactivity timeout = PT10S | ||
inactivity timeout = PT20S |
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.
What's the reason for this change?
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.
I was getting some early aborts due to this, under load, when running many tests at once. Pretty sure not related to this branch.
(All review comments addressed) |
(Those that have parents in some cycles but not in others).
Before spawn-on-demand, all tasks depended implicitly on their own previous instance submitting (same task at previous cycle point), which had several unpleasant consequences such as the inability of same-task instances to run out of order.
This fixes the last vestige of previous-instance dependence in Cylc 8: if a task instance has no parents to spawn it, we spawn it automatically when its previous instance is released from runahead. This (correctly) makes parentless tasks spawn out to the runahead limit immediately, but only if they have no parents in all cycle points. If they have parents at some points, those points temporarily block further spawning (because the parented instance does not get released from runahead until after it gets spawned by its parent).
This PR also tidies spawning-related code a lot (it was really hard to follow after my incremental shoe-horning of SOD into the old codebase ~2 yrs ago 🤦 :), and computes the runahead limit much less often.
On master,
foo
should auto-spawn to the runahead limit except for point3
where it will be spawned by3/baz
, but spawning is blocked at point3
:On this branch:
Also: close #4913
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
andconda-environment.yml
.