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 logging of task progress #4588

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bentsherman
Copy link
Member

Spun off from #4573 (comment) and below

This PR contains two improvements to the progress logging:

  • Don't show the percentage until the process is "closed", i.e. all tasks have been received. This prevents Nextflow from overestimating the progress before it knows the total task count of a process. Instead a question mark is shown.

    This requires a new lifecycle event onProcessClosed(), which is also needed to implement array jobs, task grouping, and automatic cleanup, all for the same reason -- to know how many tasks a process will execute

  • Don't include the failed task count in the percentage or the complete/total counts. This prevents the percentage from increasing when a task fails even though no progress was made, and it allows the process to reach 100% if the tasks are successfully retried/ignored.

As an example, consider a process that executes 4 tasks with 1 task that fails on the first attempt.

Before all tasks are received (i.e. process is still "open"):

[70/0de758] process > bar (3) [  ?%] 1 of 3

A task fails and is retried, and succeeds on the second attempt:

[70/0de758] process > bar (3) [ 75%] 3 of 4, retries: 1
[70/0de758] process > bar (3) [100%] 4 of 4, retries: 1 ✔

If instead the failed task is ignored, it simply reduces the number of total tasks, allowing the process to reach 100% while still noting the number of ignored tasks:

[f2/decd6c] process > bar (4) [ 75%] 2 of 3, ignored: 1
[98/47ff99] process > bar (4) [100%] 3 of 3, ignored: 1 ✔

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Copy link

netlify bot commented Dec 14, 2023

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit 112edde
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/65ca886e39fb170008f7ccfb

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Super nice 👏🏻

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman
Copy link
Member Author

Now that Phil's PR is merged, this PR is ready for review.

@bentsherman
Copy link
Member Author

@pditommaso can you review this one

@pditommaso
Copy link
Member

Not sure what's the difference closed vs terminated and how this information is showed

@bentsherman
Copy link
Member Author

Closed means the process has created all of the tasks it's going to create (except possibly for retries).

At this point we know how many tasks the process must execute and so we can show a percentage. Before this point, it makes no sense to show a percentage.

Whereas terminated means all tasks completed and the process was terminated.

See my example at the top for how I show this:

  • before the process is closed, show a question mark instead of a percentage, but still show the <completed> of <total>
  • when a task fails, don't include it in the progress because a task failure doesn't get you any closer to completion, but instead show the number of failed tasks that have been retried vs ignored

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.

None yet

3 participants