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

document graph branching with multiple optional outputs #772

Merged
merged 2 commits into from
Nov 10, 2024

Conversation

oliver-sanders
Copy link
Member

A topic which periodically comes up but is not presently documented.

See also: cylc/cylc-flow#5729

Requirements check-list

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

@oliver-sanders oliver-sanders added the content Addition or modification of documentation label Nov 7, 2024
@oliver-sanders oliver-sanders added this to the 8.3.x milestone Nov 7, 2024
@oliver-sanders oliver-sanders self-assigned this Nov 7, 2024
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.

Two minor typos.

Personal preference that I'm not super-invested in, but see what you think:

one:fail? & two:fail? => run_if_both_fail

I suppose this reduces verbosity by conveying additional information in the task name, but there's a risk that new users might think the name is magic (i.e., functional in some way) or is something other than a simple task name (it's a weird task name after all). IMO this would be better:

one:fail? & two:fail? => three  # run three if both one and two fail

src/user-guide/writing-workflows/scheduling.rst Outdated Show resolved Hide resolved
src/user-guide/writing-workflows/scheduling.rst Outdated Show resolved Hide resolved
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

Good. Please fix the spelling errors Hilary spotted.

Co-authored-by: Hilary James Oliver <[email protected]>
@oliver-sanders
Copy link
Member Author

Two minor typos.

Personal preference that I'm not super-invested in, but see what you think:

Usually these review comments go the other way, preferring concrete to abstract names.

Especially given the context, I don't think people will think these names are "magic", e.g. there's "recover" task in the previous example and an "ugly" task in the next one.

@hjoliver
Copy link
Member

hjoliver commented Nov 8, 2024

Well, "recover" conveys what the task does (/what its purpose is), which kinda makes it a normal task name. And abstract task names are fine when their purpose doesn't matter for the sake of example.

"run_if_both_fail" is different, and redundant actually, because it spells out its own triggering condition which is already evident in the graph.

Plus the other task names in the example are abstract and sensibly related to "three".

@oliver-sanders
Copy link
Member Author

(will leave you two to battle that out)

@hjoliver
Copy link
Member

hjoliver commented Nov 8, 2024

(but @wxtim hasn't given an argument one way or the other on this .. it seems you and I have to decide!)

In fact I have to retract my original statement on the pro side of long task name:

I suppose this reduces verbosity by conveying additional information

It doesn't actually do that because (as argued just above) the graph already clearly states that the task only runs if both one and two fail. So we have a long task name that re-states its own triggering condition in words - that's a bit strange isn't it? (and strange isn't good in an example - it makes users wonder what's going on)

@oliver-sanders oliver-sanders removed their assignment Nov 8, 2024
@wxtim
Copy link
Member

wxtim commented Nov 8, 2024

there's a risk that new users might think the name is magic

We're all running mental models of new users in our head, but how reliable are they?

In my opinion none of us can decide who is right on this because we are too close to the problem and we should either accept it or go and test it on a user.

@hjoliver
Copy link
Member

hjoliver commented Nov 8, 2024

[Monday] - @wxtim, I like that description but there are many kinds of user, and we can't run surveys for every little detail like this, so unfortunately we have to rely on our mental models - and if they don't align, discussion! - most of the time.

@oliver-sanders has removed his assignment from the PR, despite being the author ... I presume that reflects his oft-stated dislike for terminology-type discussions, so it's down to us. IMO I have given solid reasons for why run_if_both_fail (etc.) is not a great task name in examples, and my argument has not really been countered. However, this is just one detail in a wider issue really (consistent task names in the documentation) so to avoid holding up more important stuff I'll merge this and post an issue.,

@hjoliver hjoliver merged commit 7283354 into cylc:8.3.x Nov 10, 2024
1 check passed
@oliver-sanders oliver-sanders deleted the graph-branching++ branch November 11, 2024 09:04
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.

3 participants