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

Use unbound queue for transfer thread pool #5700

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Conversation

pditommaso
Copy link
Member

@pditommaso pditommaso commented Jan 23, 2025

This PR changes the default setting ThreadPoolBuilder class so that:

  • by default uses an unbound queue instead in order to prevent the triggering of the rejection policy when exceeding the queue size
  • use BlockingBlockingQueue when the queue size is specified
  • use the AbortPolicy as rejection policy to prevent the issue reported here
  • default to -1(unbound) queue size in the ThreadPoolManager

Solves #5363

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso pditommaso requested a review from jorgee January 23, 2025 08:53
Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit a444d5f
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/67922197ae29d00008a1090d
😎 Deploy Preview https://deploy-preview-5700--nextflow-docs-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jorgee
Copy link
Contributor

jorgee commented Jan 23, 2025

I have reproduced the error with this small pipeline that generates a directory with some files and a configuration with a small number of threads and queue size. It was failing with the master and working with this PR. I was thinking of adding it as validation or AWS test, but it could hang the CI if it fails. @pditommaso what do you think? there is a timeout for the CI build, isn't it?

main.nf

params.outdir = 's3://jorgee-eu-west1-test1/results/'
params.num_files = 10
params.file_mb = 10

process gen_data {
        publishDir "${params.outdir}", mode: 'copy'
        input:
        val(x)
        output:
        path 'data'

        script:
        """
        mkdir data
        for i in {1..${params.num_files}}
        do
            dd if=/dev/urandom of=data/chunk_${x}_\$i count=${params.file_mb} bs=1M
        done
        """
}

workflow {
        Channel.of(1) | gen_data
}

nexflow.config:

threadPool.S3TransferManager.minThreads = 1
threadPool.S3TransferManager.maxThreads = 2
threadPool.S3TransferManager.maxQueueSize = 2

@pditommaso
Copy link
Member Author

It was failing with the master and working with this PR

well done!

I was thinking of adding it as validation or AWS test, but it could hang the CI if it fails

not sure it's needed

Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso pditommaso merged commit 9031968 into master Jan 23, 2025
20 checks passed
@pditommaso pditommaso deleted the unbound-thread-pool branch January 23, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants