cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

workflow_state: reject invalid configurations

Open oliver-sanders opened this issue 1 year ago • 10 comments

  • Do not allow users to poll for transient statuses.
  • Reject invalid task states.
  • Reject polling waiting and preparing tasks (not reliably pollable).
  • Closes #6157

(minor bug as upgrade advice encouraged users to adopt invalid configurations which were not subsequently rejected)

Check List

  • [x] I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • [x] Contains logically grouped changes (else tidy your branch by rebase).
  • [x] Does not contain off-topic changes (use other PRs for other changes).
  • [x] Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • [x] Tests are included (or explain why tests are not needed).
  • [x] CHANGES.md entry included if this is a change that can affect users
  • [x] Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • [x] If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

oliver-sanders avatar Jun 27 '24 15:06 oliver-sanders

Ack, just noticed this regression:

Cylc 8.2.7:

$ cylc workflow-state Murphid -t foo -p 2001 -S catacomb --max-polls 1
InputError: invalid status 'catacomb'

Cylc 8.3.1 and this branch:

$ cylc workflow-state Murphid -t foo -p 2001 -S catacomb --max-polls 1
WARNING - --task, --status, --message, --output, --point, --task-point are deprecated. Please use an ID: Murphid//2001/foo:catacomb
ERROR - failed after 1 polls

MetRonnie avatar Jul 08 '24 15:07 MetRonnie

Ack, just noticed this regression:

Just with the deprecated -S presumably, easy fix.

oliver-sanders avatar Jul 08 '24 15:07 oliver-sanders

My bad, but need --trigger and --message --> --triggers and --messages here: https://github.com/cylc/cylc-flow/pull/6175/files#diff-16e1252035a0b4a9605f8cf4f45f5d3c4df8d90923b480137d43166beb734113R25-R26|

Link appears to be duff, where was is pointing?

oliver-sanders avatar Jul 09 '24 09:07 oliver-sanders

Why do this for the xtrigger but not the CLI?

Back compatibility?

I would prefer the CLI to be more explicit for safety too, and maybe we could make that change, but it would be an embarrassing reversal between 8.3.0 and 8.3.3.

WDYT?

oliver-sanders avatar Jul 11 '24 08:07 oliver-sanders

Would it be worth having a deprecation warning until 8.4 and then making it an InputError (either for the CLI or both CLI and xtrigger)? Although I don't think it looks great, not sure it's worse than having the xtrigger already an InputError and nothing for the CLI.

MetRonnie avatar Jul 11 '24 09:07 MetRonnie

If Hillary is ok with this I'm more happy to change the CLI behaviour.

Note that xtriggers are validated as part of workflow startup, whereas the CLI command is not validated until it is run, so it is easier to push xtrigger interface changes than.

oliver-sanders avatar Jul 11 '24 09:07 oliver-sanders

My bad, but need --trigger and --message --> --triggers and --messages here: https://github.com/cylc/cylc-flow/pull/6175/files#diff-16e1252035a0b4a9605f8cf4f45f5d3c4df8d90923b480137d43166beb734113R25-R26|

Link appears to be duff, where was is pointing?

Branch probably got rebased. ...

hjoliver avatar Jul 18 '24 02:07 hjoliver

If Hillary is ok with this I'm more happy to change the CLI behaviour.

I'm happy with that.

Note that I considered this during the main workflow-state overhaul PR, but decided against it because (a) historically we defaulted to polling for status; and (b) the command and xtrigger names also suggest that. But if y'all are happy with the change, let's make it more robust.

hjoliver avatar Jul 18 '24 02:07 hjoliver

Ok, this will cause some compat breakage, but for a better outcome. Marking this as draft until I get around to the CLI.

oliver-sanders avatar Jul 18 '24 10:07 oliver-sanders