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

Improved docker image creation #29

Merged
merged 2 commits into from
Jul 4, 2018
Merged

Improved docker image creation #29

merged 2 commits into from
Jul 4, 2018

Conversation

pditommaso
Copy link
Contributor

@pditommaso pditommaso commented Jul 2, 2018

This commit improves the image creation update the root environment instead of creating a new code environment.

The main benefit is that the installed packages are automatically added in the root environment,
therefore it’s not require to activate it or to add it to the PATH env (which it’s not a safe practice)

This commit improves the image creation update 
the root environment instead of creating a new 
code environment. 

The main benefit is that the installed packages 
are automatically added in the root environment, 
therefore it’s not require to activate it or 
to add it to the PATH env (which it’s not a safe 
practice)
@ewels ewels requested a review from a team July 2, 2018 13:59
@ewels
Copy link
Member

ewels commented Jul 2, 2018

Nice! Yes this seems cleaner. We previously had a similar change which was rejected, but I can't find it now or remember why.

Needs updates in other repos, eg. linting, for the tests to pass.

Any thoughts / better memories @sven1103 @apeltzer?

@apeltzer
Copy link
Member

apeltzer commented Jul 2, 2018

Yes, we had that already in a previous PR and then discussed a bit on this:

nf-core/chipseq#10

and the more concise discussion in here:

nf-core/cookiecutter#29

I personally think its much cleaner this way, and since we're not doing >1 environment in the containers for each workflow anyways, I suppose we could keep things as in this PR here.
We previously kind of agreed to leave things as is, but as this comes up more often now, we should potentially think about changing this to removing the environment creation and installing packages in the root directory (as in this PR).

That would require:

  • Update of the linting application
  • Change of all
    • Dockerfiles
    • Singularity

I think it makes sense to finalize the decision and my gut feeling tells me this is going towards having all packages in a single root environment, which means we'd have to change this generally...

@ewels
Copy link
Member

ewels commented Jul 2, 2018

Thanks @apeltzer! My search-foo on the iPhone during a conference is weak. Was there some issue with having a less isolated environment using root or something? I forget. But yes, I don’t have a strong opinion on this either way. If there’s a benefit for doing it one way or the other then I’m happy to change.

@apeltzer
Copy link
Member

apeltzer commented Jul 3, 2018

No problem 👍

No, as far as I can tell there was no realy issue with having a less isolated environment. Normally one wouldn't recommend doing this, but since we anyways have the "one container - one conda environment" rule, I suppose it doesn't make a difference. Except, that we can get rid of the ENV path fiddling in all of our Dockerfiles/Singularity files at once - therefore I'd also say lets switch this in general!

@maxulysse
Copy link
Member

Could that be a problem for a pipeline with multiple containers?
I should probably not do that for Sarek then...

@ewels
Copy link
Member

ewels commented Jul 4, 2018

Could that be a problem for a pipeline with multiple containers?

No, because each container still only has one conda environment in Sarek.. So no problem to do there as well.

@ewels
Copy link
Member

ewels commented Jul 4, 2018

Uck, number of downstream changes required for something simple like this gets big quickly. Need to prioritise a tool to automate this synchronisation work...

phue pushed a commit that referenced this pull request Oct 28, 2019
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.

None yet

4 participants