No spawn manual remove option

[dwsutherland]

Partially addresses #7134, supersedes #7340
Closes #7340

This PR is essentially #7340, with the command option in #7134 added (easier then rebasing the latter).

CLI doc:

  --no-spawn            Do not spawn successors before removal.
                        Warning: This is a low-level intervention for
                        targeting the spawning of parentless tasks, and may
                        result in emptying the workflow(s) of associated sub-
                        graph(s).

WUI:

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]

Thanks @dwsutherland. The command docs/warning look good, although I might think about making it even clearer.

From @oliver-sanders earlier comments:

If you want it, yes, nothing is blocking this, however, there is some outstanding work to complete

• Reload/restart flag preservation (remember this is needed to resolve the critical orphan task reload/restart bug: orphans: remove orphaned tasks on reload/restart #7209)
• Back compatibility: just as with as the cylc trigger --on-resume option before it, adding a new option creates an inter-version compatibility issue, easy to fix, just needs a compat shim.

First point (flag) done 👍 still need the shim?

[hjoliver]

Partly my bad for expanding the main CLI help paragraph (sorry), but on reflection I think we should move it all into the option help. As a low-level intervention, it does not really warrant so much space in the main command description.

Instead just leave something like this: "If you need to stop parentless tasks from spawning into future cycles, see the --no-spawn option." (Then the option help explains what that means, exactly).

[dwsutherland]

I think we should move it all into the option help

Done (I think)

[hjoliver]

Still need the back-compat shim @dwsutherland ?

Back compatibility: just as with as the cylc trigger --on-resume option before it, adding a new option creates an inter-version compatibility issue, easy to fix, just needs a compat shim.

[dwsutherland]

Still need the back-compat shim @dwsutherland ?

Back compatibility: just as with as the cylc trigger --on-resume option before it, adding a new option creates an inter-version compatibility issue, easy to fix, just needs a compat shim.

Ah yes.

[dwsutherland]

Still need the back-compat shim @dwsutherland ?

Back compatibility: just as with as the cylc trigger --on-resume option before it, adding a new option creates an inter-version compatibility issue, easy to fix, just needs a compat shim.

Ah yes.

Been trying to do this, little tricky because the UIS passes mutations straight to the scheduler.. Should have this up soon.

[dwsutherland]

Ok best I could do is handle CLI/direct-to-scheduler-api use of --no-spawn.. So 8.7 works with 8.6.x, and the reverse works too:

For the UIS, it's a bit too tricky, because we simply pass the GraphQL document on to the schedulers... And deconstructing and reconstructing the document is a little too involved..

Also if 8.7 UI passes a default remove (no --no-spawn) to a 8.6.x scheduler, then it still works fine (as the mutation is constructed without the argument in the GraphQL document).

If --no-spawn is selected, 8.7 UI to 8.6.x scheduler, then the remove isn't actioned and you get an informative error message:

So I'd say this is good enough, thoughts?

[oliver-sanders]

We're good so long as:

• An 8.7 GUI can still remove tasks in 8.6.x workflows WITHOUT the new option (obvs will fail if the flag is set).
• An 8.6.x GUI can still remove tasks in 8.7 workflows (the new option should default as off).

FYI: I think this change is saving us: cylc/cylc-ui#2230

(Gotta feeling this wasn't in place when the --on-resume compat issue occurred)

[dwsutherland]

We're good so long as:

• An 8.7 GUI can still remove tasks in 8.6.x workflows WITHOUT the new option (obvs will fail if the flag is set).

Confirmed above, this works.

• An 8.6.x GUI can still remove tasks in 8.7 workflows (the new option should default as off).

before remove:

after remove:

Still works.

FYI: I think this change is saving us: cylc/cylc-ui#2230

(Gotta feeling this wasn't in place when the --on-resume compat issue occurred)

Ah yes, nice, I assumed it was just constructing the doc from the ground up (from selected not defaults)... Glad!

[dwsutherland]

WRT coverage, it was passing 100% but now not hitting one line:

but the only way to hit this would be to create a fake pclient or start a workflow at 8.6.x... will have another look, but it was easy to test by hand.

[oliver-sanders]

Could create a fake client as you say, I think something like this would do it:

def test_whatever(monkeypath):
    client = Mock()
    client.cylc_version = ...
    monkyepatch.setattr('cylc.flow.scripts.remove.get_client', lambda *a: client)
    await remove(...)
    assert client.call_async.called_with(args, kwargs)

See also the (rather crude) way Tui amends its queries by str.replace'ing lines of it!

The proper answer (not expecting this PR to implement it!) would probably be to do something similar to what we've done in the GUI and only specify the options that we are actually using.

[oliver-sanders]

Basic string comparisons don't work right because versions are not lexigraphically sortable.

But there is an easy way of comparing versions reliably, see this example from Tui:

cylc-flow/cylc/flow/tui/data.py

Lines 126 to 136 in 4766137

	# BACK COMPAT
	# isRetry, isWallclock and isXtriggered fields added at 8.5.0
	# FROM 8.4
	# REMOVE AT: 8.8
	SpecifierSet('>=8, <8.5'),
	_QUERY
	.replace('isRetry', '')
	.replace('isWallclock', '')
	.replace('isXtriggered', '')
	.replace('estimatedFinishTime', ''),
	),

[oliver-sanders]

We should raise an error here if the user did try to specify the option rather than running the command without it (which would produce no error).

(note the Scheduler will respond with an error if unexpected args are provided, so I think we wouldn't need this compat loigic if the mutation was constructed dynamically, but double check this!)

[dwsutherland]

We should raise an error

We could do that if options.no_spawn is True, but even if it is False, that section is to make sure we don't have a GraphQL document that includes the argument for older versions.

[hjoliver]

Approving, with one minor CLI help text tweak suggested.

[hjoliver]

This now says what the option affects without saying exactly what it does. I'll just grab the salient sentence from the schema doc:

Suggested change

	help="""This only affects leading instances of parentless sequential
	xtriggered tasks and parentless tasks waiting at the runahead-limit.
	WARNING: this is a low-level intervention that cuts tasks from
	the future graph; it could cause your workflow to shut down
	prematurely as complete.
	help="""Remove the leading instance of a parentless task
	without spawning its next instance. This only affects leading instances
	of parentless sequential xtriggered tasks and parentless tasks waiting
	at the runahead-limit. WARNING: this is a low-level intervention that
	cuts tasks from the future graph; it could cause your workflow to shut
	down prematurely as complete.