-
Notifications
You must be signed in to change notification settings - Fork 651
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 description of workflow.failOnIgnore #5119
Conversation
Signed-off-by: James A. Fellows Yates <[email protected]>
✅ Deploy Preview for nextflow-docs-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -1780,7 +1780,8 @@ The `workflow` scope provides workflow execution options. | |||
`workflow.failOnIgnore` | |||
: :::{versionadded} 24.05.0-edge | |||
::: | |||
: When `true`, the pipeline will exit with a non-zero exit code if any failed tasks are ignored using the `ignore` error strategy. | |||
: When `true`, the pipeline will exit with a non-zero exit code if any failed tasks are ignored using the `ignore` error strategy. | |||
: This '`fail`' differs from the default `fail` `errorStrategy` in that the former allows all other possible tasks to be executed and complete before the pipeline fails, whereas the latter will prevent any new tasks from being started from the point of the failed task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is correct. The main (and only) difference is that that pipeline completes with a -1
exit status instead of 0
, there's no change how the tasks are executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think that confirms my point. To me with the current docs it's not clear exactly what the behaviour is 😬. If the exit code is justb different, what is the difference with the standard terminate
erorrStrategy?
Would it be better to say something like:
'Allows the ignore
errorSrategy to be respected, but still indicates a pipeline failure at the end of the run'?
My understanding of the discussions on slack was the combination of ignore and this new option (with ignore) was to allow the pipeline to otherwise 'get as far as it can' rather than killing the entire run...
If just the exit code is different, the purpose of this should be described, otherwise I don't think it's very clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively this guidance could be moved to the errorStrategy
docs under ignore
with an info box saying
'if you would still like the pipeline to continue even in the presence of failed tasks, but still ultimately have the pipeline fail at the end, see workflow.failOnIgnore
'
Or something similar.
I guess that make more sense as the docs on this current page appears to be more terse.
Would that make more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the difference with the standard terminate erorrStrategy?
terminate
: the execution is stopped as soon as an error is encountered. ignore
: any process error is just ignore, the execution continues until there's something to be run, the workflow execution completes with 0
exit status. When setting failOnIgnore=true
, the workflow execution completes with -1
.
Think it's fine keep it here better specifying the -1
exit status for the workflow execution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamrtalbot @pinin4fjords And I came to a similar conclusion independently now, to leave this as it's but better describe the use case (with the different options you described above) but in the erorrStrategy page, linking back here. Will close this over Adam's PR he will make in a bit 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may actually need further improvement based on the nf-core slack thread here, however at a bare minimum I feel this should provide sufficient better understanding of the exact purpose/behaviour of this option without being too verbose or lengthy.