Skip to content

Conversation

@bentsherman
Copy link
Member

@bentsherman bentsherman commented Apr 11, 2023

Closes #2852

TODO:

  • e2e test

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman requested a review from pditommaso April 11, 2023 19:24
@bentsherman
Copy link
Member Author

CI tests were double triggered for push and pull_request, @ewels did we do something wrong?

@ewels
Copy link
Member

ewels commented Apr 11, 2023

It's because your fork is out of date. I think if you hit "Update branch" it should work as expected... 🤞🏻

@ewels
Copy link
Member

ewels commented Apr 11, 2023

Sorry, couldn't resist hitting the button to find out 😆

Looking good now though!

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

Looking at #2118 , I believe this check was added as a partial solution:

if( len<100 )
delete << "rm -f ${Escape.path(stageName)}"

So can we safely remove this check? Now it will be in the .command.stage script anyway.

@bentsherman bentsherman linked an issue Apr 12, 2023 that may be closed by this pull request
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman requested a review from pditommaso April 14, 2023 14:09
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso
Copy link
Member

pditommaso commented Apr 18, 2023

Made some improvements:

  • write files using newBufferedWriter to avoid allocation a new byte[] buffer that for large file can be expensive
  • use bash instead of source to read the stage command
  • use NXF_WRAPPER_STAGE_FILE_THRESHOLD as bytes threshold instead of num of files to better control it

It should be added an integration tests in tests/ folder before merging it

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman requested a review from pditommaso April 18, 2023 14:38
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@bentsherman bentsherman requested a review from pditommaso April 18, 2023 15:59
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso
Copy link
Member

Considering the Slurm limit is 5MB (see here), i've creased the default to 1 MB

@pditommaso
Copy link
Member

Let's merge this. Solve #2852

@pditommaso pditommaso merged commit 4343e33 into master Apr 19, 2023
@pditommaso pditommaso deleted the 2852-separate-stage-file branch April 19, 2023 07:31
@ewels
Copy link
Member

ewels commented Apr 19, 2023

abhi18av pushed a commit to abhi18av/nextflow that referenced this pull request Oct 28, 2023
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
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.

Slurm Sbatch file too large SLURM / Sbatch File Name too long

4 participants