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

Run mode not conserved on restart #5781

Closed
wxtim opened this issue Oct 20, 2023 · 9 comments · Fixed by #5789
Closed

Run mode not conserved on restart #5781

wxtim opened this issue Oct 20, 2023 · 9 comments · Fixed by #5789
Assignees
Labels
bug Something is wrong :(
Milestone

Comments

@wxtim
Copy link
Member

wxtim commented Oct 20, 2023

check that restarting a workflow that has been running in simulation mode will restart it in simulation mode without having to specify it?
Oliver said trying to restart in simulation mode when it had been running in live mode merely restarted in live mode.

When I tried this on master it looks like it restarts in simulation mode (on master & my branch). Ick.

That's a bug.

Originally posted by @wxtim in #5712 (comment)

Example

# flow.cylc
[meta]
    purpose = """
        run this workflow with --mode live or no mode argument.

        This workflow tests whether restart maintains run mode.
        To demonstrate that task bar is run in sim mode either:

        * Check for log files for job bar (which sim mode doesn't make)
        * Add code such as

            tasks_log_list = '\n * '.join([tdef.name for tdef in taskdefs])
            LOG.debug(f'setting up {sim_mode} for tasks:\n {tasks_log_list}')

          to the method `configure_sim_modes`.
    """

[scheduler]
    allow implicit tasks = True

[scheduling]
    [[graph]]
        R1 = foo => bar

[runtime]
    [[root]]
        execution time limit = PT30S
        script = """
            sleep 20
            echo "RAN FOR REAL"
        """
    [[foo]]
        script = """
            cylc stop "${CYLC_WORKFLOW_ID}" --now --now
            while [[ ! $(grep 'INFO - DONE' "$CYLC_WORKFLOW_RUN_DIR/log/scheduler/log") ]]; do
                sleep 2
            done
            cylc play "${CYLC_WORKFLOW_ID}" --mode simulation
        """
        [[[meta]]]
            plain english = stop the workflow, and restart it in simulation mode
@wxtim wxtim self-assigned this Oct 20, 2023
@wxtim wxtim added the bug Something is wrong :( label Oct 20, 2023
@wxtim wxtim added this to the cylc-8.3.0 milestone Oct 20, 2023
@wxtim wxtim changed the title > check that restarting a workflow that has been running in simulation mode will restart it in simulation mode without having to specify it? Run mode not conserved on restart Oct 20, 2023
@MetRonnie
Copy link
Member

MetRonnie commented Oct 20, 2023

I think this issue needs a bit of clarification. There are two cases we need to consider:

  • Starting a workflow in simulation mode; stopping it; restarting it without specifying a mode
    • What should the behaviour be?
  • Starting a workflow in live mode; stopping it; restarting it with --mode=simulation
    • If it restarts in live mode that's a bug

@wxtim
Copy link
Member Author

wxtim commented Oct 20, 2023

Starting a workflow in simulation mode; stopping it; restarting it without specifying a mode

I think the run mode should conserve across restarts.

@hjoliver
Copy link
Member

hjoliver commented Oct 23, 2023

  • Starting a workflow in simulation mode; stopping it; restarting it without specifying a mode

    • What should the behaviour be?

I agree with @wxtim - it should restart in sim mode

  • Starting a workflow in live mode; stopping it; restarting it with --mode=simulation

    • If it restarts in live mode that's a bug

Agreed.

@wxtim
Copy link
Member Author

wxtim commented Oct 24, 2023

I ended up with this table:

+--------------------+--------+-------+-------+
| ↓restart \ start → | live   | sim   | dummy |
+====================+========+=======+=======+
| live               | live   | sim * | ???   |
| sim                | live * | sim   | ???   |
| dummy              | ???    | ???   | dummy |
+--------------------+--------+-------+-------+


* A warning ought to be emitted, since we're discarding user input.

Questions -

  • Do we want to test Dummy mode restarts too?
  • Should we warn or hard-fail if users set a --mode=? which we won't respect? I'm not totally convinced, but I think that you could argue that if it's a mistake it's easy enough for the user to press up in their terminal and delete the unwanted switch, and if it's intentional it's worth forcing the user to realize that it won't work.

@hjoliver
Copy link
Member

Should we warn or hard-fail if users set a --mode=?

hard fail

@hjoliver
Copy link
Member

IMO same goes for dummy mode as for sim mode. They're really the same thing, with respect to live mode. Neither one runs real tasks that generate real outputs, which is what matters if you want to restart in a different mode.

@MetRonnie
Copy link
Member

Some options like --icp cause the cylc play command to exit 1 on restart with a suitable error message. Are you proposing doing the same thing for --mode?

@wxtim
Copy link
Member Author

wxtim commented Oct 24, 2023

Some options like --icp cause the cylc play command to exit 1 on restart with a suitable error message. Are you proposing doing the same thing for --mode?

Yes. I think it's much nicer than carrying on doing something the user has every reason not to expect.

@wxtim
Copy link
Member Author

wxtim commented Oct 25, 2023

However, @hjoliver - does it make sense to stop people restarting in a different mode if we're going to allow them to override live mode by broadcasting to tasks? Perhaps it should just be a warning?

I also wonder if it would make sense for this to be a warning at 8.2.x and an exception at 8.3, since it's a fair change in behaviour?

Also, we want branches restarted without a --mode set (default value None) to pick up the database run mode, if only for auto restart - we certainly don't want auto restart to turn them into live mode.

@wxtim wxtim closed this as completed Dec 11, 2023
@MetRonnie MetRonnie linked a pull request Dec 12, 2023 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants