cylc / cylc-flow

Cylc: a workflow engine for cycling systems.
https://cylc.github.io
GNU General Public License v3.0
326 stars 90 forks source link

tui: expand cycles/families in mutations to the visible selection #4246

Open oliver-sanders opened 3 years ago

oliver-sanders commented 3 years ago

See https://github.com/cylc/cylc-flow/pull/4238#pullrequestreview-677164798

UI sibling: https://github.com/cylc/cylc-ui/issues/686

Task pool commands (e.g. cylc hold) can only apply pattern matching to the n=0 window. This means that cylc hold <family>.<cycle> or cylc hold '*.<cycle>' don't work if <cycle> is not in the n=0 window.

The practical upshot of this is if you try to hold a family or cycle of tasks in Tui that are in the n>0 window, then nothing happens.

To get around this problem we should expand the cycle/family to the full list of tasks visible in the UI.

MetRonnie commented 3 years ago

As it stands in #4238, <family>.<cycle> does work for tasks outside the n=0 window (provided the task exists at the cycle point in the graph)

oliver-sanders commented 3 years ago

Same for *.<cycle>, it's that this doesn't work outside the n=0 window (by design) that's the problem.

For example:

In this case Tui would show tasks a.1, a.2, however, attempting to hold either the cycle or family would only hold a.0 because that is the only task in the n=0 window.

cylc hold 'A.1'  # holds a.1
cylc hold '*.1'  # holds a.1

Because Tui knows what it's displaying it should issue a mutation that represents what the user is seeing (i.e. a.1, a.2 rather than 1.* or A.1). I think this is a nicer implementation in general, it isolates the user from changes which occur between the issue of the command in Tui and the command being processed in the workflow ensuring the principle of least surprise is maintained.

oliver-sanders commented 3 years ago

Adding the bug label as this is likely to cause confusion.

hjoliver commented 2 years ago

Documented deficiency, before implementation.

oliver-sanders commented 2 years ago

(try to get in as a bugfix pre 8.0.0, note the cylc-ui counterpart)

dwsutherland commented 2 years ago

Because Tui knows what it's displaying it should issue a mutation that represents what the user is seeing (i.e. a.1, a.2 rather than 1.* or A.1).

Perhaps the pool should check with the data-store? This would be a first, WRT the pool relying on the data-store for information (instead of just providing info to it)

So, the mutation will change both the existing pool and the data-store. Then when a task is first spawned (to the hidden pool), it checks the data-store for held status.. The advantage being; the store is a representation of what's visible.

The disadvantage; we'd need to ensure restarts preserve this held state?

The only other option would be a broadcast style command (which maybe it still could be), that effects both the future pool and current data-store...

oliver-sanders commented 2 years ago

Perhaps the pool should check with the data-store?

I put up a related issue, intended really for CLI use as a way for users to determine task globs would expand to - https://github.com/cylc/cylc-flow/issues/4565

However, I don't think this is necessary for Tui / UI.

The issue is that the selection of tasks the user sees on the display does not necessarily match the tasks in the pool.

If the user sees:

1/
    a
    b
    c

And issues a mutation on 1/ they would expect it to act on 1/a, 1/b & 1/c even if 1/c isn't in the pool.

The tasks the user sees are affected by the n-window (which could be set client-side) and filtering (which is set client side) so the best place to expand the selection is client side.

Tui can inspect the tree to determine what the user has selected and act on that selection. This is how multiple selection works in the Cylc 7 GUI, the code walks the tree to determine what's selected.

hjoliver commented 11 months ago

Not sure if the above covers family or pattern matching future tasks via the CLI (not just TUI and web UI)?

cylc hold <future-cycle>/FAM

Presumably we could figure out what the matched tasks or family members will be, and put those in the future hold list.

@ColemanTom provided an interesting use case https://cylc.discourse.group/t/holding-families-that-do-not-exist-in-the-task-pool/725/2 - if I hold a family that currently has some but not all members active, I probably want the other members to be held too when they appear.

oliver-sanders commented 11 months ago

Not sure if the above covers family or pattern matching future tasks via the CLI

No. This issue is only about Tui/GUI where users may run operations on a tree of tasks (e.g. cycle or family), but those operations will only apply to n=0 tasks in that tree which is confusing. The easiest way around this problem is to expand the visible selection (i.e. the tasks the user has selected in the UI).

Presumably we could figure out what the matched tasks or family members will be, and put those in the future hold list.

The suggestion of expanding families is orthogonal and was previously rejected due to issues with multiple flows and graph branching.

It is possible of course, but would require more thought, for consistency it should apply to all of the "task glob" commands, but we would need to think about how that would interact with cylc reset, there may be a housekeeping problem with graph branching (if an action is run against a task which isn't run, then we will accumulate messages causing a slow memory leak unless housekept), and if expanding families, why not cycles?

hjoliver commented 11 months ago

Presumably we could figure out what the matched tasks or family members will be, and put those in the future hold list.

The suggestion of expanding families is orthogonal and was previously rejected due to issues with multiple flows and graph branching.

Switching this to a new issue (since this is Tui/GUI only)... #5695