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

Some doc updates for 8.3.0.dev changes. #727

Merged
merged 10 commits into from
Jun 13, 2024

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented May 1, 2024

A quick blast through the docs (mostly the glossary) whilst trying not to overlap too much with the optional-outputs-extension, which should be documented separately (mostly by pasting from the proposal).

(I did add a glossary item on output completion, needed for cross-referencing).

note on "incomplete"

We agreed not to define an "incomplete task" as a finished task with incomplete outputs (because that made "incomplete" different to "not complete" ).

Now, "completion" is tied entirely to "output completion". That means it is legit to say that a task is incomplete if its outputs are incomplete, whether or not it has finished. That's convenient and I've used it a couple of times here. E.g. it means we can say a stall occurs if there is nothing left to run but *there are any incomplete tasks in the n=0 window" - that covers both finished-with-incomplete-outputs and waiting-with-partially-satisfied-prerequisites. Both of those are "active tasks" (i.e., in n=0) and have incomplete outputs.

Requirements check-list

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

@hjoliver hjoliver added this to the 8.3.0 milestone May 1, 2024
@hjoliver hjoliver force-pushed the terminology-tweaks branch 3 times, most recently from b97dea4 to 827bdff Compare May 1, 2024 06:02
@oliver-sanders
Copy link
Member

task is incomplete if its outputs are incomplete, whether or not it has finished

As per cylc/cylc-admin#191 (and related discussion), there are a couple of issues with using "finished" in this way:

  • It's not technically correct as "finished" infers that the task has started which incorrectly excludes "submit-failed" and "expired".
  • It conflicts with the ":finished" qualifier which is correctly defined as succeeded or failed.

Suggest continuing with "task with final status" or simply "final task" for this purpose.


a stall occurs if there is nothing left to run but there are any incomplete tasks in the n=0 window

Unfortunately, there problems with this statement:

  • Now, "completion" is tied entirely to "output completion", the phrase "incomplete tasks" includes active tasks. So this definition infers that a workflow can stall whilst tasks are still queued, preparing, submitted or running which is incorrect.
  • Tasks with unsatisfied prerequites may actually have complete outputs (it is possible for a task not to have any required outputs at all) so "incomplete task" does not cover "task with partially satisfied prerequisites".
  • Tasks waiting on xtriggers or extriggers may have incomplete outputs, but should not stall a workflow.

So "incomplete tasks" are not sufficient to define workflow stall or completion without also involving final task status.

(There are also a couple of small omissions, due to held tasks and the paused workflow status).


Now, "completion" is entirely to "output completion". That means it is legit to say that a task is incomplete if its outputs are incomplete

We are ok with brining back the phrase "incomplete task" in this manner (there's no issue with the term or definition), however, the term isn't especially useful as we usually simply refer to them as "waiting tasks". I'm not sure we would find uses for this new term other than in combination with "final task status".

The much more important term is "incomplete task with a final status" for which you've used, but not defined "finished incomplete task" (which has the issues outlined above). We still prefer the original definition of "incomplete task" (including status), but failing that (ground already conceded), could we have "final incomplete task" as a defined term? We need to refer to this a lot, we also need a task event for it (cylc/cylc-flow#4957), so could really do with a defined name or phrase.

@hjoliver
Copy link
Member Author

task is incomplete if its outputs are incomplete, whether or not it has finished

there are a couple of issues with using "finished" in this way: ...
... Suggest continuing with "task with final status" or simply "final task" for this purpose.

I meant "finished" in the sense that the task has finished its automatic evolution within the workflow. Nothing else is gonna happen, short of manual intervention. That covers submit-failed and expired too.

However, I agree the semantic conflict with the finished pseudo-output is a problem.

"final task" sounds like the last task in the graph, so that won't fly.

So I guess we'll have to go with "final status".

@hjoliver
Copy link
Member Author

hjoliver commented May 23, 2024

a stall occurs if there is nothing left to run but there are any incomplete tasks in the n=0 window

Unfortunately, there problems with this statement:

Yeah, sorry, I didn't really intend that statement in the description, as opposed to the docs, to exhaust all the possibilities. It was implicit (in my mind at least :-) that nothing is currently running either.

However good point on tasks with no required outputs.

@hjoliver
Copy link
Member Author

We are ok with brining back the phrase "incomplete task" in this manner (there's no issue with the term or definition), however, the term isn't especially useful as we usually simply refer to them as "waiting tasks".

No, it's not just waiting tasks - also submitted and running ones that haven't completed their outputs yet.

@hjoliver
Copy link
Member Author

hjoliver commented May 24, 2024

We still prefer the original definition of "incomplete task" (including status), but failing that (ground already conceded), could we have "final incomplete task" as a defined term?

We've covered that ground, as you say. I do think that "incomplete task" as we originally used it (final status with incomplete outputs) was nice in isolation, but in the wider context it didn't make any sense that a task wasn't incomplete until it achieved final status - so what on earth was its "completion status" prior to becoming incomplete? (I'm not even going to say "in my opinion" on that one, because it is a self-evident truth!).

could we have "final incomplete task" as a defined term? We need to refer to this a lot, we also need a task event for it (cylc/cylc-flow#4957), so could really do with a defined name or phrase.

Yes, it would be good to have a concise term for the concept "incomplete task with final status". It's a pity that we can't use "finished" as I meant it above, because of the pseudo output, as "finished incomplete" is pretty intuitive and concise.

I don't like "final incomplete task" much because it sounds like "the last incomplete task".

Perhaps we could invent a new term, divorced from all the baggage that comes with the words "final" and "complete"?

(I'll fix up the text in this PR to use the full definition for now).

@hjoliver hjoliver force-pushed the terminology-tweaks branch 2 times, most recently from bf350c8 to ca1705d Compare May 24, 2024 09:59
@hjoliver
Copy link
Member Author

@oliver-sanders - I've amended the text to fix up "finished" and "final status" as discussed above. The build fail is a link-check.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

[from the OP]

E.g. it means we can say a stall occurs if there is nothing left to run but *there are any incomplete tasks in the n=0 window"

As noted, unfortunately, this is not true. Suggest not re-adding this term (not especially useful).

It might make it easier to define "final incomplete" though as this is a top-level concept that users need to be aware of.

src/glossary.rst Outdated Show resolved Hide resolved
src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
src/user-guide/writing-workflows/scheduling.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
src/user-guide/running-workflows/reflow.rst Outdated Show resolved Hide resolved
src/user-guide/running-workflows/reflow.rst Outdated Show resolved Hide resolved
src/user-guide/running-workflows/workflow-completion.rst Outdated Show resolved Hide resolved
src/user-guide/running-workflows/workflow-completion.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

hjoliver commented Jun 11, 2024

Hopefully good to go now @oliver-sanders

Builds with cylc/cylc-flow#5809

@hjoliver
Copy link
Member Author

The final small commit tweaks the docs for workflow-state changes cylc/cylc-flow#5809

src/user-guide/writing-workflows/external-triggers.rst Outdated Show resolved Hide resolved
@@ -402,6 +395,7 @@ success. The function signature is:
.. automodule:: cylc.flow.xtriggers.xrandom
:members: xrandom, validate
:member-order: bysource
:noindex:
Copy link
Member

Choose a reason for hiding this comment

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

This is causing

Warning, treated as error:
~/cylc-doc/src/user-guide/writing-workflows/external-triggers.rst:349:py:mod reference target not found: cylc.flow.xtriggers.xrandom

Copy link
Member Author

Choose a reason for hiding this comment

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

huh, I added that to get rid of duplicate warnings ... removed again.

Copy link
Member Author

Choose a reason for hiding this comment

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

which has indeed returned:

/home/oliverh/cylc/cylc-doc/venv/lib/python3.8/site-packages/cylc/flow/xtriggers/xrandom.py:docstring
     of cylc.flow.xtriggers.xrandom:1: 
WARNING: duplicate object description of cylc.flow.xtriggers.xrandom, 
    other instance in plugins/xtriggers/built-in/cylc.flow.xtriggers.xrandom, use :noindex: for one of them

Copy link
Member

Choose a reason for hiding this comment

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

I don't get that when building

Copy link
Member

Choose a reason for hiding this comment

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

Different Sphinx versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably. So let's just ignore the warning in my env for now.
(Note if I add :noindex: my warning goes away, but then @MetRonnie gets the one above that I don't get!)

Copy link
Member

Choose a reason for hiding this comment

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

I can't see any duplicate autodoc of cylc.flow.xtriggers.xrandom

Copy link
Member

Choose a reason for hiding this comment

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

A duplicate index will cause a Sphinx error (which is what the no-index fiag is for).

src/7-to-8/major-changes/compatibility-mode.rst Outdated Show resolved Hide resolved
src/user-guide/running-workflows/workflow-completion.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

(just minor stuff)

src/glossary.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
src/workflows/xtrigger/workflow_state/downstream/flow.cylc Outdated Show resolved Hide resolved
src/workflows/xtrigger/sequential/flow.cylc Outdated Show resolved Hide resolved
src/user-guide/writing-workflows/scheduling.rst Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member Author

The one build problem is fixed on cylc/cylc-flow#5809

 <unknown>:1556: WARNING: term not in glossary: 'incomplete'

src/glossary.rst Outdated Show resolved Hide resolved
src/glossary.rst Outdated Show resolved Hide resolved
hjoliver and others added 2 commits June 13, 2024 22:18
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@oliver-sanders
Copy link
Member

@hjoliver hjoliver merged commit cde851e into cylc:master Jun 13, 2024
1 check failed
@hjoliver hjoliver deleted the terminology-tweaks branch June 13, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants