Flow trigger CLI and defaults

[hjoliver]

These changes close #4686. Built on top of - and now replaces - #4738

Implement final agreed CLI and defaults for flow triggering.

TBD

• currently spawns on all outputs of the triggered task on continuing the flow; use actual completed outputs
• make it work even if the same task is triggered multiple times
• tests for --flow=INT

Requirements 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.

• Appropriate tests are included (unit and/or functional).

• Appropriate change log entry included.

• (master branch) I have opened a documentation PR at Redo the User Guide Reflow section. cylc-doc#427

[hjoliver]

Fast tests will fail till #4746 goes in.

[hjoliver]

This fixes and extends the DB task_outputs table. On master, the table holds only custom outputs, for the most recent submit of each task. Here, it holds all outputs, with flow numbers. E.g.:

$ sqlite3 -header ~/cylc-run/ft/runN/log/db "select * from task_outputs;"
cycle|name|flow_nums|outputs
1|foo|[1]|["submitted", "started", "succeeded"]
1|bar|[1]|["submitted", "started", "the quick brown fox", "succeeded"]
1|baz|[1]|["submitted", "started", "failed"]

For repeat-running a task in the same flow via cylc trigger --wait we care about outputs from the most recent run in the right flow, not necessarily the most recent submit. So at this stage I don't think it's necessary to add submit number to the table as well.

[hjoliver]

(One last TODO for tomorrow: correctly handle n=0 flow merge with a still-running trigger --wait task)

DONE

[hjoliver]

(Rebased and deconflicted)

[oliver-sanders]

Reviewing both parts together, only one issue found so far, the trigger mutation is broken in Tui and needs a small metadata change.

Here's a PR that adds a default value to the flow arg to allow tui to work with the mutation:

hjoliver#21

And a UI PR to fix a bug that caused the flow arg to be registered as required (in the amended schema):

cylc/cylc-ui#982

[hjoliver]

Here's a PR that adds a default value to the flow arg to allow tui to work with the mutation:

Thanks, merged!

[oliver-sanders]

Most the way through.

[hjoliver]

Feedback addressed.

[dwsutherland]

Here's a PR that adds a default value to the flow arg to allow tui to work with the mutation:

Thanks, merged!

Doesn't look like it's merged..

[hjoliver]

Here's a PR that adds a default value to the flow arg to allow tui to work with the mutation:

Thanks, merged!

Doesn't look like it's merged..

Oops, good spot. It is now. I must have been waiting for tests or something.

[dwsutherland]

Here's a PR that adds a default value to the flow arg to allow tui to work with the mutation:

Thanks, merged!

Doesn't look like it's merged..

Oops, good spot. It is now. I must have been waiting for tests or something.

Rightly so, it appears, Looks like there's a style hick-up over the f-docstring:

./cylc/flow/network/schema.py:14[10](https://github.com/cylc/cylc-flow/runs/5968017405?check_suite_focus=true#step:8:10):5: B021 f-string used as docstring.This will be interpreted by python as a joined string rather than a docstring.
Error: Process completed with exit code 1.

You wouldn't mind rebasing also? so I don't have to pip install (the master changes)

(though I've done my testing, so all good either way)

[hjoliver]

Rebased.

[hjoliver]

(Damn it, tests all passed before I merged that side PR and rebased!)

[hjoliver]

Imported a flake8-bugbear error from @oliver-sanders side-PR:

./cylc/flow/network/schema.py:1410:5: B021 f-string used as docstring.This will be interpreted by python as a joined string rather than a docstring.

FYI @oliver-sanders (and others) - doc-strings cannot be f-strings:

https://docs.python.org/3/reference/lexical_analysis.html#formatted-string-literals

[hjoliver]

All fixed 🎉

[dwsutherland]

Looks good to me:

• Worked in my environment.
• Tests passed.
• Code is fine.

👍

[dwsutherland]

Happy to leave those variables unexpanded?

[hjoliver]

We can't expand them, or the docstring ceases to be a docstring (see my comment on that).

I think it's fine to leave like that. Docstrings describe code, so it's not unreasonable for them to refer to variable (or constant) names. I should perhaps have removed the braces, but I added to comment to note that it shouldn't be turned back into an f-string.

[oliver-sanders]

FYI @oliver-sanders (and others) - doc-strings cannot be f-strings:

TIL. You can modify, format or even replace a docstring after it has been declared, however, you cannot do so at the time of declaration.

FOO = 42


class Modified:
    '{FOO}'
    

Modified.__doc__ = Modified.__doc__.format(**globals())


class FString:
    f'''{FOO}'''
    
    
class Format:
    '''{FOO}'''.format(**globals())


# this works
print(Modified.__doc__)

# these don't
print(FString.__doc__)
print(Format.__doc__)

$ python mess.py 
42
None
None

[oliver-sanders]

Note: confirmed the tests from #4651 now all pass on this branch.

[oliver-sanders]

Completed functional review, all good except one bug which can cause multiple new flows to be created for a single trigger --flow=new event.

Put up a fix: hjoliver#22

Otherwise good!

[oliver-sanders]

🎉

Thanks @hjoliver this has been a fun one!

[hjoliver]

tada

Thanks @hjoliver this has been a fun one!

Heheh, yes it has 🤣

🎉