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.

Sign up for GitHub

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

Jump to bottom

Adding support for Sarus container engine #3470

Merged
merged 25 commits into from
Dec 6, 2022
Merged

Adding support for Sarus container engine #3470

merged 25 commits into from
Dec 6, 2022

Conversation

marcodelapierre
Copy link
Contributor

Hi @pditommaso @evanfloden ,

I hope this feature can be useful for part of the community.
Sarus is a container engine developed and in use at CSCS in Switzerland, see https://sarus.readthedocs.io.

I have successfully run make test locally with this branch.
I have also successfully tested the branch in action with the nextflow-io/hello workflow (both plain workflow, and some modifications, to test all options for the new feature).

@Madeeks , @lichinka ,
pinging you, so that you are aware of this new interface to Sarus being proposed for the Nextflow workflow engine, and also in case you have any feedback for this PR.

marcodelapierre and others added 20 commits December 5, 2022 21:53
@marcodelapierre
added Sarus to docs for config and containers
a336e19 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
adding more Sarus options to config docs
9650693 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
first bunch of code edits for Sarus containers
e4de6ce 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
Sarus containers, config docs: removed remove option
0440a96 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
adding SarusBuilder.groovy for Sarus containers
b3a2ab2 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
first bunch of test code edits for Sarus containers
61dcb04 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
editing BashWrapperBuilderTest.groovy for Sarus
5195c52 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
added SarusBuilderTest.groovy for Sarus
9bdd9f1 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
typo fix in SarusBuilder.groovy
ef62d30 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
typo fixes in SarusBuilderTest.groovy
8bd155d 
test fixes to SarusBuilder.groovy and SarusBuilderTest.groovy

one more test fix in SarusBuilderTest.groovy

tiny test fix in SarusBuilderTest.groovy

adding engineOptions to SarusBuilder.groovy

Revert "adding engineOptions to SarusBuilder.groovy"

This reverts commit 77f1294.

more test fixes in SarusBuilderTest.groovy

more test fixes in SarusBuilder and SarusBuilderTest

more SarusBuilderTest

Revert "more SarusBuilderTest"

This reverts commit 88f50c1.

one more fix to SarusBuilderTest

one moooooooore fix to SarusBuilderTest

yet another fix to SarusBuilderTest

Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
small add to container doc
e7f6c23 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@ATpoint @marcodelapierre
Add sentence on bash options (nextflow-io#3454) [ci skip]
3bf7cb6 
Signed-off-by: Alexander Toenges <21218078+ATpoint@users.noreply.github.com>
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@aaronegolden @pditommaso @marcodelapierre
Fix a few issues in BatchLogging.groovy (nextflow-io#3443)
12c1018 
1. Adds the projectId to BatchLogging's LoggingOptions so the logs
will be fetched from the correct project. Previously I got a lot of
"that resource might not exist" responses from the log read requests.
I'm not sure if there's something special about my environment that
causes me to need the projectId in the options, but I think it
couldn't hurt to have the projectId in general.

2. Removes the old parseOutput function, which relied on the STDERR and
STDOUT prefixes in the payload of the log entries. Batch no longer adds
prefixes so that parsing doesn't work. Now we can distinguish stderr
from stdout by looking at the logEntry's severity.

3. Catches any exception thrown by the log reading code. It could be a
permissions issue like I was seeing, or a thread pool issuee as in
nextflow-io#3166. Either way,
something going wrong while trying to read task logs should probably
not stop the whole workflow.

With these changes, the nf-core/methylseq workflow can be run with
the google-batch executor.

Signed-off-by: Aaron Golden <aegolden@google.com>

Signed-off-by: Aaron Golden <aegolden@google.com>
Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@aaronegolden @marcodelapierre
Fix Quote the logName in the Cloud Logging filter (nextflow-io#3464)
77a061f 
Quoting will allow special characters, in particular ":", to be
parsed. This should prevent the unparseable filter exception in
nextflow-io#3353.

Signed-off-by: Aaron Golden <aegolden@google.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@pditommaso @marcodelapierre
Add warning on Google Logs failure [ci fast]
8feeda1 
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@nathanthorpe @pditommaso @marcodelapierre
Rewrite fetchIamRole and fetchRegion to use AWS SDK (nextflow-io#3425)…
f4592c8 
… [ci skip]

Signed-off-by: Nathan Thorpe <nathan@techstormpc.com>
Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@pditommaso @marcodelapierre
Bump AWS sdk version 1.12.351
9aad766 
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@pditommaso @marcodelapierre
Update docs [ci skip]
72b3a4a 
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@pditommaso @marcodelapierre
Remove deprecated code
0beab7c 
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
sarus: output of pull to stderr
722fea2 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
Copy link
Contributor Author

note: the commit list looks a bit weird to me, compared to PRs I usually open.
I had to do a sign-off rebase due to missing commit signatures, which is what caused the mixing between my commits and recent commits by others.

@pditommaso
Copy link
Member

You are a star!

@pditommaso
Copy link
Member

pditommaso commented Dec 5, 2022 •
edited
Loading

Can Sarun pull the image on-the-fly as it can be done with docker pull or it does need to bepulled ahead of the run?

@marcodelapierre
Copy link
Contributor Author

Can Sarun pull the image on-the-fly as it can be done with docker pull or it does need to bepulled ahead of the run?

Unfortunately, at the moment automatic pull is not support in Sarus (see eth-cscs/sarus#6)

@marcodelapierre
sarus: edit to author/copyright, 2x files
4184539 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
@marcodelapierre
comment edit in BashWrapperBuilder
06cfa2b 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
pditommaso
pditommaso reviewed Dec 5, 2022
View reviewed changes
docs/container.rst Outdated Show resolved Hide resolved
@pditommaso
Update docs/container.rst [ci skip]
8b75d60 
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
@pditommaso
Copy link
Member

If the automatic pull then this mention in the docs looks inconsistent to me or am I wrong?

https://github.com/nextflow-io/nextflow/pull/3470/files#diff-22c9ee42ec9ef1d2237cadf42e6d24114e3f0fd8d50b98299fa1f13fcbb76853R351

@Madeeks
Copy link

Madeeks commented Dec 5, 2022

Can Sarun pull the image on-the-fly as it can be done with docker pull or it does need to bepulled ahead of the run?

Unfortunately, at the moment automatic pull is not support in Sarus (see eth-cscs/sarus#6)

Hi @marcodelapierre @pditommaso, thanks for your interest in Sarus and for this development!
I just wanted to make some quick comments.

One of the motivations for not including an automatic image pull in sarus run is to avoid multiple concurrent pulls if a user launches a large scale job without having the image available. This is not a technical limitation, Sarus has atomic update mechanisms for images and repository metadata which work quite well, but it's mainly a way to prevent time/resource waste and encourage a "pull once, deploy at scale" usage model, for which Sarus was originally conceived.

Since Sarus 1.5.0, sarus pull returns immediately if the requested image is already available locally, so prepending a pull before a run should have negligible wall clock time impact after the first invocation. Apologies for not mentioning this in the related issue.

@pditommaso
Copy link
Member

Hi @Madeeks thanks for the heads up! this is indeed very interesting.

I think adding the pull before the runs would make the experience more consistent with the expectation of nextflow users. What do you think @marcodelapierre?

@marcodelapierre
Copy link
Contributor Author

Thanks @Madeeks and @pditommaso for your feedback! :-)

Indeed, I am already pre-pulling images to be consistent with the Nextflow user experience: https://github.com/marcodelapierre/nextflow/blob/add/sarus_containers/modules/nextflow/src/main/groovy/nextflow/container/SarusBuilder.groovy#L84-L92

@pditommaso pditommaso linked an issue Dec 6, 2022 that may be closed by this pull request
Support for Sarus container runtime #2648
Closed
2 tasks
@pditommaso
Copy link
Member

Works like a charm!

Screenshot 2022-12-06 at 16 22 09

@pditommaso pditommaso merged commit 54673f1 into nextflow-io:master Dec 6, 2022
@marcodelapierre marcodelapierre deleted the add/sarus_containers branch December 6, 2022 22:40
l-modolo pushed a commit to l-modolo/nextflow that referenced this pull request Jan 25, 2023
@marcodelapierre @l-modolo
Add support for Sarus container engine (nextflow-io#3470)
0c518a5 
Signed-off-by: Marco De La Pierre <marco.delapierre@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pditommaso pditommaso pditommaso left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for Sarus container runtime
6 participants
@marcodelapierre @pditommaso @Madeeks @ATpoint @aaronegolden @nathanthorpe