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 spawning of absolute-triggered tasks. #5008

Merged
merged 3 commits into from
Jul 25, 2022

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 24, 2022

These changes close #5002

I've run the example out to 50 cycle points several times, with nothing strange happening

I haven't thought of a reliable way to reproduce the problem (which was more of an inefficiency + confusing warning than a balls-to-the-wall bug) in a short test. But codecov says the change is covered in a strictly technical sense at least.

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).
  • Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • Already covered by existing tests.
  • Appropriate change log entry included.
  • No documentation update required.

@hjoliver hjoliver added the bug Something is wrong :( label Jul 24, 2022
@hjoliver hjoliver added this to the cylc-8.0.0 milestone Jul 24, 2022
@hjoliver hjoliver self-assigned this Jul 24, 2022
@hjoliver hjoliver requested a review from MetRonnie July 24, 2022 11:15
@hjoliver hjoliver marked this pull request as ready for review July 24, 2022 11:47
@hjoliver
Copy link
Member Author

Explanation:

Consider a task foo that depends only on an absolute trigger:

P1 = "init[3] => foo"

In principle, init[3] should spawn all foo instances, but that's not practically possible (there could be a large of infinite number of foo instances). So we have to autospawn foo instances as they were parentless, but make sure that none of them run before init[3] succeeds, or not spawn any foo until init[3] succeeds.

On current master, we spawn 3/foo after init:succeed, and then autospawn other foo instances (out to the runahead limit) as if they are parentless. However, that's tricky and has an edge case problem.

On this branch, we autospawn all foo immediately (to the runahead limit), but they stay in the hidden pool (which holds partially satisfied prerequisites) with an unsatisfied prerequisite on init:succeed, until that gets satisfied.

This is simpler, and it works in all cases.

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

Was able to reproduce:

(flow) sutherlander@graphic-vbox:~$ cylc cat-log parentless | grep WARNING
2022-07-25T15:00:07+12:00 WARNING - Backward compatibility mode ON
2022-07-25T15:00:14+12:00 WARNING - Task 3/foo already spawned in {1}
2022-07-25T15:00:14+12:00 WARNING - Task 5/foo already spawned in {1}
2022-07-25T15:00:14+12:00 WARNING - Task 5/foo already spawned in {1}
2022-07-25T15:00:57+12:00 WARNING - Task 23/foo already spawned in {1}

Fixed after this change:

(flow) sutherlander@graphic-vbox:~$ cylc cat-log parentless | grep WARNING
2022-07-25T15:02:59+12:00 WARNING - Backward compatibility mode ON

Code change looks fine, & tests passing 👍

@wxtim wxtim requested review from wxtim and removed request for MetRonnie July 25, 2022 08:38
@wxtim
Copy link
Member

wxtim commented Jul 25, 2022

Booted @MetRonnie as reviewer because I was able to reproduce issue locally.

Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Read the code
  • Checked out and run tests locally
  • Run Local Proof of pudding tests up to cycle 50

@wxtim wxtim merged commit 734d6a0 into cylc:master Jul 25, 2022
Comment on lines -50 to +52
'R1/2': 'foo[1] => pub' # 2/pub doesn't spawn at start
'R1/2': 'foo[1] => pub'
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a task to this test that doesn't get spawned to the runahead limit? (That was the point of pub originally, I think)

Copy link
Member Author

@hjoliver hjoliver Jul 25, 2022

Choose a reason for hiding this comment

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

Yes, I'd like to spend a bit of time making these integration tests more comprehensive. As a quick fix, I just adapted the existing test to what's actually going on in the scheduler now.

Copy link
Member Author

Choose a reason for hiding this comment

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

New issue: #5012

@hjoliver hjoliver deleted the fix-spawning-issue branch July 25, 2022 09:52
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.

Parentless tasks try to spawn again after already finishing
4 participants