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

Fix workflow-state command and xtrigger. #5809

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Nov 3, 2023


Minor db change Change to the workflow database structure : Format of outputs column in task_outputs table has changed from Cylc 8.0.0 where it was a stringified list of task messages, to a stringified dictionary of the format {<output label>: <task message>} (like how it was in Cylc 7).


The command and xtrigger now take WORKFLOW//CYCLE/TASK:SELECTOR as an arg.

SELECTOR can be a status or an output. With --output, or if not a valid status, it is taken as an output; otherwise a status.

Any query will be polled for if no matching result is found.

A good test case:

[scheduling]
    cycling mode = integer
    initial cycle point = 2001
    final cycle point = 2003
[scheduling]
    [[xtriggers]]
        ws = workflow_state("%(workflow)s//%(point)s/foo:succeeded", offset="-P1"):PT2S
    [[graph]]
        P1 = """
           foo[-P1] => foo
           foo:x => bar & baz
        """
        R/2002/P1 = "@ws => baz"
[runtime]
    [[foo]]
        script = """
           cylc message -- "xxx"
        """
        [[[outputs]]]
           x = xxx
    [[bar]]
        script = """
          if [[ "$CYLC_TASK_CYCLE_POINT" == "$CYLC_WORKFLOW_FINAL_CYCLE_POINT" \
             && "$CYLC_TASK_SUBMIT_NUMBER" == "1" ]]
          then
              cylc trigger --flow=new $CYLC_WORKFLOW_ID//2002/foo
          fi
        """
    [[baz]]
$ cylc workflow-state demo
demo/run1
2001/foo:succeeded
2001/bar:succeeded
2001/baz:succeeded
2002/foo:succeeded
2002/bar:succeeded
2002/baz:succeeded
2003/foo:succeeded
2003/bar:succeeded
2003/baz:succeeded
2002/foo:succeeded(flows=2)
2002/bar:succeeded(flows=2)
2002/baz:succeeded(flows=2)
2003/foo:succeeded(flows=2)
2003/bar:succeeded(flows=2)
2003/baz:succeeded(flows=2)

$ cylc workflow-state demo//2001
demo/run1
2001/foo:succeeded
2001/bar:succeeded
2001/baz:succeeded

$ cylc workflow-state demo//2001/foo:x
demo/run1
2001/foo:['submitted', 'started', 'succeeded', 'x'

$ cylc workflow-state "demo//*/*:x"
demo/run1
2001/foo:['submitted', 'started', 'succeeded', 'x']
2002/foo:['submitted', 'started', 'succeeded', 'x']
2003/foo:['submitted', 'started', 'succeeded', 'x']
2002/foo:['submitted', 'started', 'succeeded', 'x'](flows=2)
2003/foo:['submitted', 'started', 'succeeded', 'x'](flows=2)

$ cylc workflow-state demo --flow=2
demo/run1
2002/foo:succeeded(flows=2)
2002/bar:succeeded(flows=2)
2002/baz:succeeded(flows=2)
2003/foo:succeeded(flows=2)
2003/bar:succeeded(flows=2)
2003/baz:succeeded(flows=2)

$ cylc workflow-state demo --flow=2 --output
demo/run1
2002/foo:['submitted', 'started', 'succeeded', 'x'](flows=2)
2002/bar:['submitted', 'started', 'succeeded'](flows=2)
2002/baz:['submitted', 'started', 'succeeded'](flows=2)
2003/foo:['submitted', 'started', 'succeeded', 'x'](flows=2)
2003/bar:['submitted', 'started', 'succeeded'](flows=2)
2003/baz:['submitted', 'started', 'succeeded'](flows=2)

$ cylc workflow-state "demo//2001/b*"
demo/run1
2001/bar:succeeded
2001/baz:succeeded

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).
  • CHANGES.md 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 label Nov 3, 2023
@hjoliver hjoliver added this to the cylc-8.3.0 milestone Nov 3, 2023
@hjoliver hjoliver self-assigned this Nov 3, 2023
@hjoliver hjoliver marked this pull request as ready for review November 6, 2023 10:36
changes.d/5809.fix.d Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/dbstatecheck.py Outdated Show resolved Hide resolved
tests/unit/xtriggers/test_workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

Note also: #6030

@MetRonnie MetRonnie removed their request for review April 9, 2024 13:52
@hjoliver hjoliver marked this pull request as draft April 19, 2024 03:01
@hjoliver hjoliver modified the milestones: 8.4.0, 8.3.0 Apr 19, 2024
@hjoliver hjoliver mentioned this pull request Apr 19, 2024
8 tasks
@hjoliver hjoliver force-pushed the fix-workflow-state-check branch 2 times, most recently from 113942b to 697681f Compare April 26, 2024 11:36
@hjoliver hjoliver marked this pull request as ready for review April 26, 2024 11:48
@MetRonnie MetRonnie self-requested a review April 29, 2024 13:31
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/scripts/workflow_state.py Outdated Show resolved Hide resolved
cylc/flow/dbstatecheck.py Outdated Show resolved Hide resolved
cylc/flow/dbstatecheck.py Outdated Show resolved Hide resolved
tests/functional/xtriggers/04-suite_state.t Outdated Show resolved Hide resolved
cylc/flow/xtrigger_mgr.py Show resolved Hide resolved
@MetRonnie
Copy link
Member

tests/k/xtriggers/01-workflow_state.t failing btw

@hjoliver hjoliver marked this pull request as draft April 30, 2024 07:49
cylc/flow/dbstatecheck.py Outdated Show resolved Hide resolved
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

Seems reasonable; Given the amount on your plate I will start drafting tests for this so that we can ram it through.

I've added the following:hjoliver#50 - It's not a complete set of tests, but I've covered some of this. I'll do more tomorrow. If you want to get them in feel free to merge though.

cylc/flow/dbstatecheck.py Outdated Show resolved Hide resolved
cylc/flow/dbstatecheck.py Outdated Show resolved Hide resolved
cylc/flow/config.py Outdated Show resolved Hide resolved
@wxtim
Copy link
Member

wxtim commented May 31, 2024

I ran a workflow on master, stopped it and restarted on your branch - the DB looks like this: Is that OK?

1291|foo|[1]|["submitted", "started", "succeeded", "common message"]
1292|foo|[1]|{"submitted": "submitted", "started": "started", "succeeded": "succeeded", "selector2": "common message"}

@hjoliver
Copy link
Member Author

hjoliver commented Jun 3, 2024

I ran a workflow on master, stopped it and restarted on your branch - the DB looks like this: Is that OK?

1291|foo|[1]|["submitted", "started", "succeeded", "common message"]
1292|foo|[1]|{"submitted": "submitted", "started": "started", "succeeded": "succeeded", "selector2": "common message"}

Yes, on master the DB stores task messages as "outputs". And in Cylc 7, only custom outputs were stored. We are now to say the trigger name is the "output", not the corresponding task message.

For maximum flexibility and back compat we need to store both (trigger name and task message).

@MetRonnie
Copy link
Member

MetRonnie commented Jun 4, 2024

$ cylc workflow-state gimli --output 'and my axe'
InputError: Please give a single ID

Not an especially clear error message for someone using the old CLI syntax.

Also:

$ cylc workflow-state gimli/          
Traceback (most recent call last):
  File "~/.conda/envs/cylc8/bin/cylc", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "~/github/cylc-flow/cylc/flow/scripts/cylc.py", line 703, in main
    execute_cmd(command, *cmd_args)
  File "~/github/cylc-flow/cylc/flow/scripts/cylc.py", line 334, in execute_cmd
    entry_point.load()(*args)
  File "~/github/cylc-flow/cylc/flow/terminal.py", line 282, in wrapper
    wrapped_function(*wrapped_args, **wrapped_kwargs)
  File "~/github/cylc-flow/cylc/flow/scripts/workflow_state.py", line 307, in main
    poller = WorkflowPoller(
             ^^^^^^^^^^^^^^^
  File "~/github/cylc-flow/cylc/flow/scripts/workflow_state.py", line 158, in __init__
    tokens = Tokens(self.id_)
             ^^^^^^^^^^^^^^^^
  File "~/github/cylc-flow/cylc/flow/id.py", line 111, in __init__
    kwargs = tokenise(args[0], relative)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/github/cylc-flow/cylc/flow/id.py", line 673, in tokenise
    raise ValueError(f'Invalid Cylc identifier: {identifier}')
ValueError: Invalid Cylc identifier: gimli/

Lost the stripping of the trailing slash that comes from bash standard autocomplete

@hjoliver
Copy link
Member Author

hjoliver commented Jun 5, 2024

$ cylc workflow-state gimli --output 'and my axe'
InputError: Please give a single ID

Not an especially clear error message for someone using the old CLI syntax.

That's because the option has become a flag, to interpret the task ID selector as an output (trigger) as opposed to a status or a task message.

I suppose I could change the names of these flags to --outputs and --messages, to avoid this. Since multiple outputs will be returned if they match the command line.

@wxtim wxtim self-requested a review June 5, 2024 09:13
@MetRonnie
Copy link
Member

MetRonnie commented Jun 5, 2024

You mentioned transient statueses in the videoconf today. This currently does not work once the task status is suceeded:

workflow_state(gimli//1/foo:submitted)

For a Cylc 7 DB we have no way around this other than the user adding is_output=True to the function call.

But for Cylc 8 DBs we can just query the task_outputs table instead, no need to check the task_states table at all, right? (Oh, maybe for a task that has been cylc set...)

@hjoliver
Copy link
Member Author

hjoliver commented Jun 5, 2024

For consistency with previous usage, and with the command name "workflow-state", I've stuck with interpreting standard names ("submitted" etc.) as statuses by default, but now you can choose to query the corresponding output instead (with --output) to avoid the transient status problem. If you do that, then the task_states table is not checked at all.

In an earlier iteration of this branch I did in fact automatically translate transient states to outputs, but bailed out of that for the reasons above, and because it could potentially be confusing in the case of running (status) --> started (output) - might look like getting something you didn't ask for.

Is that what you're asking?

@MetRonnie
Copy link
Member

MetRonnie commented Jun 5, 2024

Ah right, I forgot that statuses include "running", "waiting" etc. Never mind.

Btw, I've checked that cylc set --output does result in the output label: message getting added to the task_outputs table row 👍

@MetRonnie MetRonnie added the db change Change to the workflow database structure label Jun 5, 2024
@hjoliver
Copy link
Member Author

hjoliver commented Jun 7, 2024

Tests all passing again, this time with manual output completion recorded in the DB. There's still a bit more to do though:

  • trawl through the comments above
  • see if I can make the CLI back-compatible so as not to break existing hand-rolled polling tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug config change Involves a change to global or workflow config db change Change to the workflow database structure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

workflow-state: check latest instance for status
4 participants