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

Re-implement edit-run #3751

Closed
hjoliver opened this issue Aug 5, 2020 · 13 comments
Closed

Re-implement edit-run #3751

hjoliver opened this issue Aug 5, 2020 · 13 comments

Comments

@hjoliver
Copy link
Member

hjoliver commented Aug 5, 2020

cylc trigger --edit was removed during SoD implementation. This is not really a "sod-follow-up" issue however (although I'm labeling it as such for now) because edit-run needs to be completely redone for Cylc 8 anyway, and for that it is a cross-component problem: cylc-flow, cylc-UI, and cylc-UIServer . It's not clear what needs to be done for this in cylc-flow yet.

@hjoliver hjoliver added this to the cylc-8.0.0 milestone Aug 5, 2020
@oliver-sanders
Copy link
Member

I think we need to make the job file available in the GraphQL schema to that the clients can access it without having to go to the filesystem. Then the client sends a diff along with the trigger request to the scheduler.

This will be a bit tricky for the UI as the forms are auto-generated and this involves an extra step so the diff field should use a custom type (extending string) to give the UI a fighting chance.

@hjoliver
Copy link
Member Author

My notes don't seem to have captured the Feb workshop discussion on this, but I think we were also considering presenting the parsed config via the UI, for edit-run capability, instead of the raw file. That could allow authorization control over different config items, for example. It would be presumably be a more complex solution to implement though.

@oliver-sanders
Copy link
Member

Since the job file is now just a bunch of functions called by the real job script (which you can't edit) there is very little value to being able to edit the actual job script itself. Being able to edit the script items and environment vars should suffice for all use cases, we can achieve this as broadcast+trigger.

@hjoliver
Copy link
Member Author

I must be misunderstanding you, as that seems to contradict your previous comment?? ("we need to make the job file available in the GraphQL schema to that the clients can access it", and then "there is very little value to being able to edit the actual job script itself.")

Being able to edit the script items and environment vars should suffice for all use cases,

That's what I was suggesting - present the separate job config items, separately. Potentially then, we could allow the owner to edit any of those (mainly script and environment) and authorized others to only change environment variables (or perhaps even only some environment variables, but they'd have to be marked in the flow config somehow).

@oliver-sanders
Copy link
Member

I must be misunderstanding you

Yes.

In my second commend I'm suggesting that perhaps trigger-edit functionality does not need to be re-implemented, just use broadcast+trigger under the hood.

This would require exposing the [runtime] config via GraphQL rather than the job file.

@hjoliver
Copy link
Member Author

Alright, thanks for explaining - agreed then 🎉

@oliver-sanders
Copy link
Member

oliver-sanders commented Mar 3, 2021

Given this a little thought, two options for implementation using the query+broadcast model:

Option 1 - use the .cylc format as the interface

  1. UI requests the config for a task.
  2. UIS serves the lineralised (family expanded) task config as plain text (.cylc).
  3. User edits this in the UI.
  4. The UI sends thed edited task config back to the UIS as plain text (.cylc).
  5. UIS runs this through the configuration parser (Python).
  6. UIS determines what has changed and issues a broadcast with those settings.

E.G. if this is the flow.cylc:

[runtime]
    [[A]]
        pre-script = abc
    [[a]]
        inherit = A
        script = def

And the user tries to trigger edit a.1 (1) then the UIS would return this to this UI (2):

# NOTE1: this must come from the Scheduler as existing broadcasts must be applied
# NOTE2: this config is linearised, inheritance is expanded, there is no inherit configuration
pre-script = abc
script = def

The user then edits this (3) and submits it to the UIS (4):

# edited config
pre-script = abc
script = defghi
[environment]
    FOO = bar

The UIS runs this through the configuration parser (5). This is needed to properly interpret things like Cylc multi-line syntax """, line continuations, overrides, etc.

  • I think we may be able to parse and validate the task configuration as a fragment by parsing it against the relevant section of the SPEC, but if we do that we get Python objects out rather than the raw strings the user input. This will require more investigation.
  • We will need to reject any change which attempts to set the inherit configuration.

The UIS determines what the edit has changed and issues a broadcast via GraphQL (6). Should be equivalent to the following CLI command:

$ broadcast <flow> \
    -n <task name> \
    -p <cycle point> \
    -s script=defghi \
    -s [environment]FOO=bar

Option 2 - use a GraphQL interface

A variant of option 1. Rather than providing the task config in .cylc format we provide it as a list of key=value pairs.

  • Would make other configurations available via the GraphQL interface which may be useful elsewhere:
    query {
       workflow {
          config {
               runtime {
                   name
                   script
                   environment {
                       key
                       value
      ...
  • Would require us to hard-code the "full" query for all relevant configs/sub-configs and keep that up to date.
  • Removes the need for Cylc config parsing.
  • If we want validation we would need to re-implement it here.

Notes

  • As it stands the broadcast that the trigger edit makes would live for the life of the task so would apply to future submissions.
    • This is a change from Cylc7 behaviour.
    • We could implement a mechanism in cylc broadcast to target specific job submissions.
    • Or we could call it a feature, this information will be visible in the "broadcast view".
  • Trigger edits could be configured to effect future cycles.
    • Would be quite useful to trigger edit for this and all future occurences.
    • Suggest adding the cycle point glob argument from the "broadcast" mutation.
  • There is no need for a multi-task trigger edit, use a regular broadcast.

@hjoliver hjoliver modified the milestones: cylc-8.0.0, cylc-8.x Aug 4, 2021
@oliver-sanders
Copy link
Member

Especially if choosing option (2), suggest using Tui to provide the trigger-edit interface on the CLI.

@oliver-sanders
Copy link
Member

Note, we will need to apply active broadcasts to the configuration before returning the expanded configuration.

@oliver-sanders
Copy link
Member

I've spun off the [runtime] config query part of this issue into - #5054

@MetRonnie
Copy link
Member

I have opened a UI issue for this: cylc/cylc-ui#1104

@hjoliver
Copy link
Member Author

@hjoliver hjoliver removed this from the cylc-8.x milestone Oct 17, 2022
@ColemanTom
Copy link
Contributor

I know this is closed, but I think https://cylc.github.io/cylc-doc/latest/html/7-to-8/caveats.html#cylc-flow needs updating?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants