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

Add run mode skip to cylc show #6554

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

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jan 13, 2025

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.
  • Very, very small feature.

@wxtim wxtim force-pushed the feat.skip_shown_in_cylc_show branch from 04bd7b1 to b01e496 Compare January 13, 2025 15:37
@wxtim wxtim changed the base branch from master to 8.4.x January 13, 2025 15:37
@wxtim wxtim self-assigned this Jan 13, 2025
@wxtim wxtim added the could be better Not exactly a bug, but not ideal. label Jan 13, 2025
cylc/flow/scripts/show.py Outdated Show resolved Hide resolved
cylc/flow/scripts/show.py Outdated Show resolved Hide resolved
@MetRonnie MetRonnie marked this pull request as draft January 13, 2025 16:07
@wxtim wxtim requested a review from oliver-sanders January 15, 2025 11:54
@wxtim wxtim marked this pull request as ready for review January 15, 2025 11:54
@wxtim wxtim force-pushed the feat.skip_shown_in_cylc_show branch from bfa3254 to 71e3f59 Compare January 16, 2025 11:25
tests/integration/scripts/test_show.py Outdated Show resolved Hide resolved
tests/integration/scripts/test_show.py Outdated Show resolved Hide resolved
cylc/flow/scripts/show.py Outdated Show resolved Hide resolved
@wxtim wxtim force-pushed the feat.skip_shown_in_cylc_show branch from 9c2f350 to 060a20a Compare January 17, 2025 08:17
@wxtim wxtim requested a review from oliver-sanders January 17, 2025 08:17
Comment on lines 634 to 639
TaskRunMode = graphene.Enum(
'TaskRunMode',
[(m.capitalize(), m) for m in TASK_CONFIG_RUN_MODES],
[(k.capitalize(), k.lower()) for k in RunMode.__members__.keys()],
# [(m.capitalize(), m) for m in TASK_CONFIG_RUN_MODES],
description=lambda x: RunMode(x.value).describe() if x else None,
)
Copy link
Member

Choose a reason for hiding this comment

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

This should be fine because TaskRunMode is only used for data (queries / subscriptions) and not in arguments or mutations.

However, the "Edit Runtime" feature uses this enum to generate the run-mode dropdown and then feeds that value into a broadcast mutation 🤦.

Screenshot from 2025-01-20 15-21-49

That broadcast will then fail if it is provided an invalid value (though the user doesn't appear to be notified of this from the GUI):

ERROR - (type=option) run mode = dummy
ERROR - graphql.error.located_error.GraphQLLocatedError: (type=option) run mode = dummy

Not a biggie, but ideally, we would find a way to subtract the invalid values from the "Edit Runtime" form.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it make more sense to keep this the way it was? Simulation and Dummy are not task run modes, they are workflow run modes.

Copy link
Member Author

@wxtim wxtim Jan 21, 2025

Choose a reason for hiding this comment

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

I think too much can be made of the distinction between task and workflow run modes; they both change the execution of individual tasks. The only difference is how the user can apply them.

@oliver-sanders - are you suggesting that I carry on with this PR, but try to put up a companion on cylc-ui to filter out the simulation and dummy?

Copy link
Member

@oliver-sanders oliver-sanders Jan 21, 2025

Choose a reason for hiding this comment

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

Doesn't it make more sense to keep this the way it was? Simulation and Dummy are not task run modes, they are workflow run modes.

No, they are definitely task run modes, we just don't let people configure them in the workflow config.

I.E, these two run modes are are readonly values set by the scheduler.

Copy link
Member

@oliver-sanders oliver-sanders Jan 21, 2025

Choose a reason for hiding this comment

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

@oliver-sanders - are you suggesting that I carry on with this PR, but try to put up a companion on cylc-ui to filter out the simulation and dummy?

Undecided. Edit runtime assumes, not unreasonably, that all possible values of an enum are also valid values for the user to set. Though this isn't actually true of other types (i.e. not all possible strings are valid inputs for a string config, some will also be rejected by broadcast).

We could probs filter out these two options by registering a custom type with the form generator which would disable the field if set to certain values? This would at least give us the opportunity to explain the situation in a tooltip.

Copy link
Member Author

@wxtim wxtim Jan 21, 2025

Choose a reason for hiding this comment

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

Right, so this can be handled in cylc show by using the workflow run mode if it's Simulation or Dummy, else falling back to the task run mode?

This was what I originally considered, but discarded, because you'd have to replicate that logic anywhere else you use run mode.

After re-reading Oliver's options I think I like 2 best, 1 next.

Copy link
Member

@MetRonnie MetRonnie Jan 22, 2025

Choose a reason for hiding this comment

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

We could cover this up... but that would still leave the "Edit Runtime" form displaying the wrong value and allowing the user to edit it (e.g. change live -> skip when the workflow is running in sim mode) even though the change would be invalid

Most things you can change in the Edit Runtime form are not valid for simulation mode! I think being able to fruitlessly choose Live/Skip mode when running in simulation mode is less of a problem than being able to fruitlessly choose Dummy/Simulation in live mode.

So if we're going down the more complicated but future-facing route, I think this ought to be in tandem with a sibling UI/UIS PR so we can make changes in either as needed

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure which way you're arguing here?

I'm leaning towards option 1: Ignore the issue, it's not a biggie.

  • The task's run_mode field should be set to the mode that the task will (or did) run in (i.e. live, skip, sim or dummy).
  • The only issue with this is with the edit runtime form.
  • But invalid broadcasts will result in an error anyway so this issue of little consequence, we just need to make sure the error is clearly presented.
  • If we were worried about this issue, we could use a custom Enum so we don't offer users run mode changes that will be rejected by broadcast anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Will discuss at the team meeting

Copy link
Member

Choose a reason for hiding this comment

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

Register a new type with the form generator that only allows this to be set to live/skip and only if the workflow mode is live.

Ok, I understand how to do this now, will open a UI PR for it shortly

@wxtim wxtim marked this pull request as draft January 21, 2025 08:45
changes.d/6554.feat.md Outdated Show resolved Hide resolved
wxtim and others added 4 commits January 22, 2025 10:14
broaden the definition of task-mode

Save workflow states to the runtime config object.
fixed TUI test

fix a broken functional test
@wxtim wxtim force-pushed the feat.skip_shown_in_cylc_show branch from 16640a8 to 4ca26a0 Compare January 22, 2025 13:44
@wxtim wxtim changed the base branch from 8.4.x to master January 22, 2025 13:44
@MetRonnie MetRonnie added this to the 8.5.0 milestone Jan 22, 2025
@MetRonnie MetRonnie added the schema change Change to the Cylc GraphQL schema label Jan 22, 2025
@wxtim wxtim marked this pull request as ready for review January 23, 2025 09:02
Comment on lines 88 to 94
@classmethod
def _missing_(cls, value):
value = value.lower()
for member in cls:
if member.lower() == value:
return member
return None
Copy link
Member

Choose a reason for hiding this comment

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

Surprised to learn about this. The fact that it's "sunder" (single underscore) is weirding me out. https://docs.python.org/3/library/enum.html#enum.Enum._missing_

wxtim and others added 2 commits January 24, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
could be better Not exactly a bug, but not ideal. schema change Change to the Cylc GraphQL schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants