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

Errant flow merge when triggering #6361

Closed
MetRonnie opened this issue Sep 6, 2024 · 9 comments · Fixed by #6367
Closed

Errant flow merge when triggering #6361

MetRonnie opened this issue Sep 6, 2024 · 9 comments · Fixed by #6367
Assignees
Labels
bug Something is wrong :(
Milestone

Comments

@MetRonnie
Copy link
Member

Description

R1 = """
    foo
    bar
"""

If foo fails in flow 1 while bar is in flow 2, triggering foo without providing a flow in the options causes it to run in flows 1 & 2.

This is because the default --flow all is causing it to pick all active flow numbers in the entire task pool instead of all active flow numbers for the task in question.

if flow[0] == FLOW_ALL:
flow_nums = self._get_active_flow_nums()

def _get_active_flow_nums(self) -> Set[int]:
"""Return active flow numbers.
If there are no active flows (e.g. on restarting a completed workflow)
return the most recent active flows.
"""
fnums = set()
for itask in self.get_tasks():
fnums.update(itask.flow_nums)
if not fnums:
fnums = self.workflow_db_mgr.pri_dao.select_latest_flow_nums()
return fnums

Looking at the code, this bug probably affects cylc set too

@MetRonnie MetRonnie added the bug Something is wrong :( label Sep 6, 2024
@MetRonnie MetRonnie added this to the 8.3.5 milestone Sep 6, 2024
@hjoliver hjoliver self-assigned this Sep 11, 2024
@oliver-sanders oliver-sanders added bug? Not sure if this is a bug or not question Flag this as a question for the next Cylc project meeting. and removed bug Something is wrong :( labels Sep 11, 2024
@oliver-sanders
Copy link
Member

If foo fails in flow 1 while bar is in flow 2, triggering foo without providing a flow in the options causes it to run in flows 1 & 2.

Hmm, there's nothing untoward here, this is the intended behaviour, all triggers default to --flow=all.

--flow=all means "all flows in the workflow" not "all flows on the task".

@oliver-sanders oliver-sanders modified the milestones: 8.3.5, 8.3.x Sep 11, 2024
@MetRonnie
Copy link
Member Author

But maybe the default should not be "all flows in the workflow", but "all flows on the task"?

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 11, 2024

If the task has already spawned, then the trigger should only apply to the spawned flows (see this line in the set proposal):

--flow=INT: Flow(s) to attribute spawned tasks. Default: all active flows.
If a task already spawned, use current flows.

-- https://github.com/cylc/cylc-admin/blob/2fa7e970926935e76758010be1798d6d709d1b47/docs/proposal-cylc-set.md?plain=1#L65-L66

I would expect this logic to extend to trigger.

If it hasn't already spawned then it should get all flows. This default was well debated at the time.

I.E. It depends on the pool state.

@MetRonnie
Copy link
Member Author

Right so this is a bug then. The default should be "all flows on the task", which is meaningless if it hasn't spawned yet, in which case it should be "all flows in the workflow"?

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 11, 2024

"all flows on the task", which is meaningless if it hasn't spawned yet

👍

So long as we're strictly talking n=0 here. The proposals already define how --flow=all should interact with an n=0 task.

@oliver-sanders oliver-sanders added bug Something is wrong :( and removed bug? Not sure if this is a bug or not question Flag this as a question for the next Cylc project meeting. labels Sep 11, 2024
@MetRonnie
Copy link
Member Author

What about setting/triggering a task that already ran and is no longer in n=0? Should it not use the flow number(s) from last time it ran?

I tested this and this is the case for n>1 in the past. However for n=1 in the past it is flow-merging like described for n=0.

@oliver-sanders
Copy link
Member

oliver-sanders commented Sep 11, 2024

What about setting/triggering a task that already ran and is no longer in n=0? Should it not use the flow number(s) from last time it ran?

I don't think there's any special logic for n<0. A historical task in one flow might be a future task in another flow so the previous run might not have any relevance to the trigger.

@hjoliver
Copy link
Member

Just to confirm, the plan was:

  • if triggering an n=0 (pool) task the default should be that it just keeps its already-assigned flow numbers

@oliver-sanders - it seems like you agree but initially did not realize that @MetRonnie was specifically talking about triggering an n=0 task?

@MetRonnie MetRonnie modified the milestones: 8.3.x, 8.3.5 Oct 2, 2024
@oliver-sanders
Copy link
Member

Closed by #6367

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 a pull request may close this issue.

3 participants