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

Add docker.runOptions to avoid memory swap error #351

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

olgabot
Copy link
Contributor

@olgabot olgabot commented Jun 25, 2019

Hello! I had been running into some docker errors and tracked down some magic settings that made them go away. This PR adds those settings to the default nf-core created sample.

PR checklist

  • This comment contains a description of changes (with reason)
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: https://github.com/nf-core/tools/tree/master/.github/CONTRIBUTING.md

@ewels
Copy link
Member

ewels commented Jun 25, 2019

I feel like we've talked about adding this to the main template before, but were nervous that it would cause problems for some users (which is why it's not already a default in nextflow). Maybe @pditommaso can remember?

Generally I tend to think that stuff like this should be addressed upstream in nextflow rather than being patched in pipelines downstream though, if possible..

@pditommaso
Copy link

Basically enforcing the user and group id changes some env variables such as USER and HOME, however, it should be just fine 99% of the cases. More details here.

Yes, the plan is to include as default. If you could stress test it a bit with nf-core pipelines and confirm there's no impact, I would be happy to make it the default option.

@ewels
Copy link
Member

ewels commented Jun 27, 2019

Ok great, thanks! Then yes maybe we can merge this PR so it (eventually) goes out to all pipelines. Would be good to add a comment that this will hopefully be a default in the future and can be removed at a later date...

// Avoid this error:
// WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
// Thanks to: https://github.com/alesssia/YAMP/wiki/How-to-use-Docker
docker.runOptions = '-u $(id -u):$(id -g)'
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Not sure that is required - the '...' should be enough if it's in nextflow.config right?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I didn't understood what you meant ;-)

Copy link
Member

Choose a reason for hiding this comment

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

I mean I'm not sure whether we need to escape the $ with a \$ here as they are inside a '...' single quotes block :)

Copy link
Member

Choose a reason for hiding this comment

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

Aaaah, now I understand.
I'm pretty sure I copied that from someone else (probably Paolo) at some point.
I'm guessing the double quotes render the escaping the $ a necessity

Copy link
Member

Choose a reason for hiding this comment

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

Then we need to address this as well - I was looking for another place where this is used but couldn't find it. Wouldn't fixOwnership as listed here do the same? https://www.nextflow.io/docs/latest/config.html#config-docker

@@ -58,6 +58,11 @@ profiles {
test { includeConfig 'conf/test.config' }
}

// Avoid this error:
// WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
// Thanks to: https://github.com/alesssia/YAMP/wiki/How-to-use-Docker
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Thanks to: https://github.com/alesssia/YAMP/wiki/How-to-use-Docker
// Thanks to: https://github.com/alesssia/YAMP/wiki/How-to-use-Docker
// Testing this in nf-core after discussion here https://github.com/nf-core/tools/pull/351, once this is established and works well, nextflow might implement this behavior as new default.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add this in @olgabot? That should be enough to be able to trace things back once they're in the template.

@apeltzer apeltzer changed the base branch from master to dockerrunOptions June 28, 2019 06:42
@apeltzer
Copy link
Member

Merging this to dockerrunOptions to make the final changes required 👍

@apeltzer apeltzer merged commit 5f1f453 into dockerrunOptions Jun 28, 2019
@apeltzer apeltzer mentioned this pull request Jun 28, 2019
5 tasks
@apeltzer apeltzer deleted the olgabot-patch-1 branch September 20, 2019 12:15
@maxibor
Copy link
Member

maxibor commented Feb 3, 2020

@apeltzer @pditommaso @ewels : -u $(id -u):$(id -g) doesn't seem to be a jack of all trades: when using the Ete toolkit python package in the coproID pipeline, with these docker run options, I get an error:

NCBI database not present yet (first time used?)
  Traceback (most recent call last):
    File "/opt/conda/envs/nf-core-coproid-1.1dev/bin/sourcepredict", line 176, in <module>
      sm.compute_distance(distance_method=distance_method, rank=RANK)
    File "/opt/conda/envs/nf-core-coproid-1.1dev/bin/sourcepredictlib/ml.py", line 257, in compute_distance
      ncbi = NCBITaxa()
    File "/opt/conda/envs/nf-core-coproid-1.1dev/lib/python3.6/site-packages/ete3/ncbi_taxonomy/ncbiquery.py", line 110, in __init__
      self.update_taxonomy_database(taxdump_file)
    File "/opt/conda/envs/nf-core-coproid-1.1dev/lib/python3.6/site-packages/ete3/ncbi_taxonomy/ncbiquery.py", line 129, in update_taxonomy_database
      update_db(self.dbfile)
    File "/opt/conda/envs/nf-core-coproid-1.1dev/lib/python3.6/site-packages/ete3/ncbi_taxonomy/ncbiquery.py", line 731, in update_db
      os.mkdir(basepath)
  PermissionError: [Errno 13] Permission denied: '/.etetoolkit'

Using the default Docker run parameters works like a charm in this context.

RHReynolds added a commit to RHReynolds/nf-core-scrnaseq that referenced this pull request Sep 21, 2021
When running dsl2 branch --profile test,docker the following error
occurs: "WARNING: Your kernel does not support swap limit capabilities
or the cgroup is not mounted. Memory limited without swap." This is
fixed by adding "docker." prior to "runOptions", as per the discussion
here: nf-core/tools#351
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.

6 participants