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

Spawn parentless sequential xtriggered task on set outputs #6448

Merged

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented Oct 24, 2024

closes #6447

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 if present).
  • Test added.
  • Changelog entry included if this is a change that can affect users
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@dwsutherland dwsutherland added bug Something is wrong :( small labels Oct 24, 2024
@dwsutherland dwsutherland added this to the 8.3.6 milestone Oct 24, 2024
@dwsutherland dwsutherland self-assigned this Oct 24, 2024
@dwsutherland
Copy link
Member Author

Will add a test (or add to existing).

@dwsutherland dwsutherland changed the title Spawn parentless sequential xtrigger on set outputs Spawn parentless sequential xtriggered task on set outputs Oct 24, 2024
@MetRonnie MetRonnie linked an issue Oct 24, 2024 that may be closed by this pull request
@MetRonnie MetRonnie self-requested a review October 24, 2024 12:25
Comment on lines 2136 to 2128
# Task may be set running before xtrigger is satisfied,
# if so check/spawn if xtrigger sequential.
elif (
itask.is_xtrigger_sequential
and (
itask.identity not in
self.xtrigger_mgr.sequential_has_spawned_next
)
):
self.xtrigger_mgr.sequential_has_spawned_next.add(
itask.identity
)
self.spawn_to_rh_limit(
itask.tdef,
itask.tdef.next_point(itask.point),
itask.flow_nums
)
else:
# Task may be set running before xtrigger is satisfied,
# if so check/spawn if xtrigger sequential.
self.check_spawn_psx_task(itask)
# De-queue it to run now.
self.task_queue_mgr.force_release_task(itask)
Copy link
Member

@MetRonnie MetRonnie Oct 24, 2024

Choose a reason for hiding this comment

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

This has changed the logic - was it meant to? Or should it be

# Task may be set running before xtrigger is satisfied,
# if so check/spawn if xtrigger sequential.
elif not self.check_spawn_psx_task(itask):
    # De-queue it to run now.
    self.task_queue_mgr.force_release_task(itask)

(having changed check_spawn_psx_task to return a boolean)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but I figured it was ok to de-queue and spawn (if psx) if not runahead released..

Copy link
Member Author

Choose a reason for hiding this comment

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

Have changed it like you said... better to keep them separate I suppose, since they are different category of action..

Copy link
Member Author

@dwsutherland dwsutherland Oct 24, 2024

Choose a reason for hiding this comment

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

Actually, strike that .. This is a force trigger .. PSX tasks should be spawned outside of this runahead/de-queue here.. So have put this outside and after the if/else block.

@dwsutherland dwsutherland force-pushed the spawn-on-set-parentless-seq-xtrig-tasks branch from 34ce7b3 to f50b365 Compare October 24, 2024 19:33
@dwsutherland dwsutherland force-pushed the spawn-on-set-parentless-seq-xtrig-tasks branch from f50b365 to edd8c86 Compare October 24, 2024 20:42
@dwsutherland dwsutherland force-pushed the spawn-on-set-parentless-seq-xtrig-tasks branch from edd8c86 to d08137b Compare October 24, 2024 20:47
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.

(From a very quick review and test - I ran out of time today, sorry - it looks good)

@MetRonnie MetRonnie merged commit ddf5349 into cylc:8.3.x Oct 29, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-spawn on set of parentless sequential xtriggered task
3 participants