Skip to content

2 remove runcript from singularity build - #4

Merged
pditommaso merged 2 commits into
masterfrom
2-remove-runcript-from-singularity-build
Sep 7, 2023
Merged

2 remove runcript from singularity build#4
pditommaso merged 2 commits into
masterfrom
2-remove-runcript-from-singularity-build

Conversation

@munishchouhan

Copy link
Copy Markdown
Member

This PR will remove %runscript from singularity image and make it simple

@pditommaso pditommaso left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

leave to @marcodelapierre to confirm

@pditommaso

Copy link
Copy Markdown
Contributor

Working in my tests, leave Marco propose any change post-merging

@pditommaso
pditommaso merged commit 932e4bc into master Sep 7, 2023
@pditommaso
pditommaso deleted the 2-remove-runcript-from-singularity-build branch September 7, 2023 16:20
@pditommaso
pditommaso restored the 2-remove-runcript-from-singularity-build branch September 7, 2023 16:21

@marcodelapierre marcodelapierre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look good, two important comments, especially the first one:

  1. If still as is, I strongly recommend using singularity exec instead of singularity run, as this is the correct, supported usage for Singularity (see singularity docs, https://docs.sylabs.io/guides/latest/user-guide/quick_start.html#executing-commands vs https://docs.sylabs.io/guides/latest/user-guide/quick_start.html#running-a-container

  2. The image hash for singularity vs docker is different, right?

@pditommaso

Copy link
Copy Markdown
Contributor

The image hash for singularity vs docker is different, right?

yes

If still as is, I strongly recommend using singularity exec instead of singularity run

The rationale was to align to the Docker entrypoint behaviour, especially for the problem of Conda initialization. See here. However, I understand that also run can have negative side effects. Let's discussion on nextflow repo

@marcodelapierre

Copy link
Copy Markdown
Contributor

Hi @pditommaso have a look at this comment of mine:

seqeralabs/wave#288 (comment).

Although I need to have a closer look at the scripts/requirements for mamba-org/micromamba-docker#155 (comment), I am pretty sure that the /.singularity.d/env/ approach would fix singularity exec and singularity shell, including that case.

It looks to me like right now Nextflow is still using exec, right? See https://github.com/nextflow-io/nextflow/blob/master/modules/nextflow/src/main/groovy/nextflow/container/SingularityBuilder.groovy#L63

So please let me know how you would best like to follow up on this, based on the extra info above.

@pditommaso

Copy link
Copy Markdown
Contributor

There has been a tentative to use run instead of exec to align it with Docker nextflow-io/nextflow@a1e1d3ca.

However I've reverted this change in the last release nextflow-io/nextflow@c1cc60dd

@marcodelapierre

Copy link
Copy Markdown
Contributor

Thanks for sharing these code refs Paolo 👍

@munishchouhan
munishchouhan deleted the 2-remove-runcript-from-singularity-build branch October 11, 2023 10:08
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.

3 participants