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

Setting default maxRetries and errorStrategy and override for some processes #5464

Open
mcolpus opened this issue Nov 4, 2024 · 3 comments · May be fixed by #5474
Open

Setting default maxRetries and errorStrategy and override for some processes #5464

mcolpus opened this issue Nov 4, 2024 · 3 comments · May be fixed by #5474
Assignees
Labels

Comments

@mcolpus
Copy link

mcolpus commented Nov 4, 2024

Bug report

(Please follow this template replacing the text between parentheses with the requested information)

Expected behavior and actual behavior

I want to set errorStrategy and maxRetries in the nextflow config, and then for some processes to change the maxRetries with the aim that the errorStrategy should adjust accordingly

Steps to reproduce the problem

main.nf:

process A {
    maxRetries 4
    input:

    script:
    """
    exit 1
    """
}

process B {
    input:

    script:
    """
    exit 1
    """
}

workflow  {
    A()
    B()
}

nextflow.config:

profiles {
    local {
        process {
            executor = 'local'
            errorStrategy = {
                sleep(5 * Math.pow(2, task.attempt) as long)
                task.attempt <= process.maxRetries ? 'retry' : 'ignore'
            }
            maxRetries = 1
        }
    }

    other {
        process {
            maxRetries = 4
            errorStrategy = {
                sleep(60 * Math.pow(2, task.attempt) as long)
                task.attempt <= process.maxRetries ? 'retry' : 'terminate'
            }
        }
    }
}

Program output

The process maxRetries is ignored by the errorStrategy.

ubuntu@mcolpus-main:~/pipelines/nextflow_test_max_retries$ nextflow main.nf -profile local

 N E X T F L O W   ~  version 24.10.0

Launching `main.nf` [stupefied_ramanujan] DSL2 - revision: d107b1c91a

executor >  local (4)
[8e/c5cee3] A [100%] 2 of 2, failed: 2, retries: 1 ✔
[14/7baa91] B [100%] 2 of 2, failed: 2, retries: 1 ✔
[16/71c57f] NOTE: Process `B` terminated with an error exit status (1) -- Execution is retried (1)
[2d/4487c6] NOTE: Process `A` terminated with an error exit status (1) -- Execution is retried (1)
[8e/c5cee3] NOTE: Process `A` terminated with an error exit status (1) -- Error is ignored
[14/7baa91] NOTE: Process `B` terminated with an error exit status (1) -- Error is ignored

If the process error strategy is lower than the configs default then it will fail and not be ignored.

I also tried used task.maxRetries instead, but that crashed.
nextflow.log

Environment

  • Nextflow version: 24.10.0
  • Java version: 17.0.12 2024-07-16
  • Operating system: linux
  • Bash version: 5.1.16(1)-release
@bentsherman
Copy link
Member

task.maxRetries is the correct way to do it in the directive closure, but it looks like there is a stack overflow

@jorgee can you take a look?

@bentsherman bentsherman added the bug label Nov 4, 2024
@jorgee jorgee self-assigned this Nov 4, 2024
@jorgee
Copy link
Contributor

jorgee commented Nov 5, 2024

The problem seems to be an infinite loop that crashes the stack. The maxRetries is checking the errorStrategy to set max retries to 0 if it is different to 'retry'. But, as defined in the process configuration, to get errorStrategy it is evaluating maxRetries, producing the loop.
https://github.com/nextflow-io/nextflow/blob/549e7902dc6203a854b4c82732a760d9a0358c14/modules/nextflow/src/main/groovy/nextflow/processor/TaskConfig.groovy#L346C1-L350C6
The errorStrategy check was added in this commit 7 years ago, but it is not used in the master branch.

In my opinion, we could set the maxRetries to 1 in all the cases without evaluating the the errorStrategy to avoid the loop. In the current nextflow code, the getMaxRetries is only called when the task fails and strategy is RETRY.

@pditommaso
Copy link
Member

Likely, not sure why there's that check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants