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

Improve migration guide #490

Merged
merged 12 commits into from
Jul 18, 2022
Merged

Improve migration guide #490

merged 12 commits into from
Jul 18, 2022

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Jul 4, 2022

Add content to and reorganise into more logical order:

  • Back compat
  • Optional outputs/suicide triggers
  • Platforms

Also update slightly incorrect CYLC_VERSION Jinja2 variable thing

Would really like to merge this by Wed 6th morning (UTC) + redeploy 8.0rc3 docs in time for workshop, sorry for the short notice!

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.

@MetRonnie MetRonnie added the content Addition or modification of documentation label Jul 4, 2022
@MetRonnie MetRonnie added this to the 8.0rc3 milestone Jul 4, 2022
@MetRonnie MetRonnie self-assigned this Jul 4, 2022
@hjoliver
Copy link
Member

hjoliver commented Jul 5, 2022

(I'll get onto this review pronto...)

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Partially reviewed; looks good so far.

src/7-to-8/major-changes/cli.rst Show resolved Hide resolved
src/7-to-8/major-changes/cli.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/cli.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
@MetRonnie MetRonnie linked an issue Jul 5, 2022 that may be closed by this pull request
Comment on lines 91 to 106
Restarting a Cylc 7 workflow
^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Cylc 8 cannot *restart* a Cylc 7 workflow mid-run. Instead, :ref:`install
<Installing-workflows>` the workflow to a new run directory and start it
from scratch at the right cycle point or task(s):

To see if your workflow is compatible with Cylc 8, run ``cylc validate``
**using Cylc 7**.
- ``cylc play --start-cycle-point=<cycle>`` (c.f. Cylc 7 *warm start*), or
- ``cylc play --start-task=<cycle/task>`` (Cylc 8 can start anywhere in the graph)

If tasks in your workflow call Cylc commands directly it might be necessary to
modify them to be compatible with command line interface changes
(unfortunately, this is not possible to detect at validation).
.. note::

Any previous-cycle workflow data needed by the new run will need to be
manually copied over from the original run directory.


Copy link
Member

Choose a reason for hiding this comment

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

I think we should drop this, continuing a Cylc 7 workflow run with Cylc 8 is a tricky use case with caveats. Easier just to say Cylc 8 cannot restart Cylc 7 workflows.

Copy link
Member

@hjoliver hjoliver Jul 7, 2022

Choose a reason for hiding this comment

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

It will be a very common requirement though. Should we not provide advice on how to do it?

Maybe don't call it "restarting", to avoid confusion. Perhaps something like How to start your migrated workflow from where it got to under Cylc 7. And give general advice and caveats.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or "Restarting" a Cylc 7 workflow?

Copy link
Member

Choose a reason for hiding this comment

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

It will be a very common requirement though.

I'm not aware of any requirement or desire at our end. I think the only use case would be really, really, long-running workflows, I'm not aware that we have any examples of this. If they exist I would advise finishing the workflow run with Cylc 7 rather than to attempting to migrate to Cylc 8 part-way through a run.

Migrating like this is hazardous, e.g. remote installation works differently, data might need to be relocated on multiple filesystems, etc. We discussed this in a meeting at our end?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've replaced this subsection with a bullet point saying you can't restart, use Cylc 7 to finish a Cylc 7 workflow

Copy link
Member

@hjoliver hjoliver Jul 18, 2022

Choose a reason for hiding this comment

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

I'm not aware of any requirement or desire at our end. I think the only use case would be really, really, long-running workflows, I'm not aware that we have any examples of this.

Most of our workflows are "really, really, long-running" - i.e. they have no final cycle point, or a very distant one. And most are "warm cycling" - i.e there are inter-cycle dependencies, and doing a cold start from scratch is not something you want to do unless you have no choice.

Anyhow, we may have got our wires crossed here. I just meant "advise users how to continue a warm-cycling Cylc 7 workflow with Cylc 8", not a literal in-place restart from the DB.

Along the lines of:

Shut down (with Cylc 7) after a particular cycle point, copy any inter-cycle data over to a new Cylc 8 install (taking account of the run-name if appropriate), and cold start that with Cylc 8 at the next cycle point.

src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/suicide-triggers.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/suicide-triggers.rst Outdated Show resolved Hide resolved
Comment on lines -47 to +49
foo => bar
foo:fail => recover

# The fail case.
bar:fail => recover
foo | recover => bar

# Remove the "recover" task in the success case.
bar => ! recover

# Remove the "bar" task in the fail case.
recover => ! bar

# Downstream dependencies.
bar | recover => baz

Which results in the following logic:
foo => ! recover
# Remove the "foo" task in the fail case.
recover => ! foo
Copy link
Member

Choose a reason for hiding this comment

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

IMO this was clearer before.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have taken out the first task because it was unnecessary for the example.

That first edge was confusingly commented as the success case, but it wasn't really, the bar => baz was the success case

graph TD
    subgraph before
    A(foo) -->|unnecessary| B(bar) -->|success case| C(baz)
    B -.->|fail case| D(recover) -.-> C
    end
Loading
graph TD
    subgraph after
    A(foo) --> B(bar)
    A -.-> C(recover) -.->B
    end
Loading

What part of it would you say was clearer before?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the first task was unnecessary. It was probably abstracted from an old example that was meant to look more realistic.

@oliver-sanders
Copy link
Member

@wxtim You may want to poke at this one because it will conflict with your platforms PR.

MetRonnie added a commit that referenced this pull request Jul 5, 2022
@MetRonnie
Copy link
Member Author

I have taken the (squashed) commits from this branch as-is and added to branch-8.0rc3 so I can re-deploy the docs. I will do that now, can always re-deploy again if any further changes are needed

@hjoliver
Copy link
Member

hjoliver commented Jul 6, 2022

I have taken the (squashed) commits from this branch as-is and added to branch-8.0rc3 so I can re-deploy the docs. I will do that now, can always re-deploy again if any further changes are needed

Ah, that explains why I was seeing your changes here, online already!! 😕

@wxtim
Copy link
Member

wxtim commented Jul 6, 2022

@wxtim You may want to poke at this one because it will conflict with your platforms PR.

No worries - my PR is small enough that I'm happy to deal with the conflict. 😄

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

LGTM!

src/7-to-8/major-changes/cli.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/cli.rst Show resolved Hide resolved
src/7-to-8/major-changes/cli.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
In Cylc 8 we mark the :term:`outputs <task output>` which are
:term:`optional <optional output>` (in this case ``bar:success`` and
``bar:fail``) with a ``?`` in the graph.
In Cylc 8, all task outputs are :term:`expected <expected output>` to complete
Copy link
Member

Choose a reason for hiding this comment

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

NB #492 s/expected/required/g

Copy link
Member

Choose a reason for hiding this comment

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

(I'll update 492 after merging this)

@hjoliver
Copy link
Member

Two approvals, and time is of the essence this week. I'll post a small follow-up. Thanks @MetRonnie

@hjoliver hjoliver merged commit 0d4028c into cylc:master Jul 18, 2022
@MetRonnie MetRonnie deleted the back-compat branch July 18, 2022 08:44
@MetRonnie
Copy link
Member Author

MetRonnie commented Jul 18, 2022

Pushed squashed commit to branch-8.0rc3 and redeploying...

Update: can't deploy right now, linkcheck failing because man.openbsd.org is down

Update 2: deployed. (I imagine this will be the last for 8.0rc3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Addition or modification of documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration guide: optional outputs
4 participants