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.

Sign up for GitHub

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

Jump to bottom

flow merge for force-triggered n=0 (e.g. queued) tasks #6241

Merged
merged 4 commits into from
Jul 19, 2024
Merged

flow merge for force-triggered n=0 (e.g. queued) tasks #6241

merged 4 commits into from
Jul 19, 2024

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jul 17, 2024 •
edited
Loading

Supersede #6232
Close #6174

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).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@hjoliver hjoliver added the bug Something is wrong :( label Jul 17, 2024
@hjoliver hjoliver added this to the 8.3.3 milestone Jul 17, 2024
@hjoliver hjoliver self-assigned this Jul 17, 2024
hjoliver
hjoliver commented Jul 17, 2024
View reviewed changes
cylc/flow/data_store_mgr.py
@@ -2367,6 2367,25 @@ def delta_task_queued(self, itask: TaskProxy) -> None:
self.state_update_families.add(tproxy.first_parent)
self.updates_pending = True

def delta_task_flow_nums(self, itask: TaskProxy) -> None:
Copy link
Member Author

Choose a reason for hiding this comment

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

There was no way to update flow_nums in the data_store.

We got away with this by (erroneously) re-adding flow-merged n=0 tasks to the task pool, which created an entirely new datastore task proxy.

hjoliver
hjoliver commented Jul 17, 2024
View reviewed changes
cylc/flow/task_pool.py
Comment on lines -1417 to 1420
self.add_to_pool(t)
if not in_pool:
self.add_to_pool(t)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the add-to-pool bug.

hjoliver
hjoliver commented Jul 17, 2024
View reviewed changes
cylc/flow/task_pool.py
@@ -2169,6 2172,7 @@ def force_trigger_tasks(
if itask.state(TASK_STATUS_PREPARING, *TASK_STATUSES_ACTIVE):
LOG.warning(f"[{itask}] ignoring trigger - already active")
continue
self.merge_flows(itask, flow_nums)
Copy link
Member Author

@hjoliver hjoliver Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is all that was needed to trigger an existing n=0 task (e.g. queued) in a new flow. The rest of the PR is just making sure the flow_nums change ends up in the data store.

@hjoliver
Copy link
Member Author

hjoliver commented Jul 17, 2024 •
edited
Loading

(One SOD test broken, expecting an easy fix though...) FIXED

@oliver-sanders oliver-sanders mentioned this pull request Jul 18, 2024
Closed
8 tasks
oliver-sanders
oliver-sanders approved these changes Jul 18, 2024
View reviewed changes
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.

👍

tests/integration/test_task_pool.py Outdated Show resolved Hide resolved
@wxtim
Copy link
Member

wxtim commented Jul 19, 2024 •
edited
Loading

Awaiting Runahead

Have replicated bug with a runahead limited task - and confirmed this PR works.

If I set the workflow running, and whilst a task is waiting because of the runahead limit I trigger it in a new flow I don't think it gets triggered in that new flow.

[scheduling]
    initial cycle point = 1000
    runahead limit = P2
    [[graph]]
        P1Y = foo
[runtime]
    [[foo]]
        script = sleep 40

> cylc vip

# Look at tui or otherwise check that 10030101T0000Z/foo is waiting on runahead

> cylc trigger workflow//10030101T0000Z/foo --flow=2

> sqlite3 ~/cylc-run/my-workflow/log/db 'select * from task_outputs'
...
10030101T0000Z|foo|[1]|{"submitted": "submitted", "started": "started", "succeeded": "succeeded"}
...

Also checked:

  • Awaiting held test
  • Custom queues

@wxtim wxtim merged commit c029d6b into cylc:8.3.x Jul 19, 2024
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxtim wxtim wxtim approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@hjoliver hjoliver

Labels
bug Something is wrong :(
Projects
None yet
Milestone
8.3.3
Development

Successfully merging this pull request may close these issues.
3 participants
@hjoliver @wxtim @oliver-sanders