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

MAINT: Stage Python environment of Docker image from nipreps/miniconda #2581

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Oct 7, 2021

This PR explores the possibility of sharing some base images across end-user preps.

Leveraging the fact that conda stuffs everything under the distribution's directory, that is particularly convenient to install all the dependencies we need related to Python and NodeJS with multi-stage builds, and also make the environment more consistent across preps.

The source of nipreps/miniconda is found at https://github.com/nipreps-containers/miniconda/blob/main/Dockerfile

I still believe that multi-stage builds are a neat solution to our build times, especially if we are going to start pruning out unused components of packages.

@oesteban oesteban requested review from effigies and mgxd October 7, 2021 14:31
@oesteban
Copy link
Member Author

oesteban commented Oct 7, 2021

cc @eilidhmacnicol I think this is the way around the problems with the docker images you were experiencing.

Comment on lines +271 to +273
ENV PATH="/opt/conda/bin:$PATH" \
CPATH="/opt/conda/include:$CPATH" \
LD_LIBRARY_PATH="/opt/conda/lib:$LD_LIBRARY_PATH" \
Copy link
Member

Choose a reason for hiding this comment

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

This is going to cause pain for people reusing nipype working directories across 21.0.0rc's. I think it's okay, but we should make it part of the release notes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I guess that's actually a good argument to push this kind of changes before the 21.0.0 release rather than postponing to 21.1.0, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it's fine to make the switch now

@eilidhmacnicol
Copy link

eilidhmacnicol commented Oct 7, 2021

cc @eilidhmacnicol I think this is the way around the problems with the docker images you were experiencing.

I'm not so sure this will help me, unfortunately. I think the issue that I was having was pulling an ubuntu image with the default call on an M1 Mac built an image with an arm64 architecture. The arm64 image had build problems before conda was installed (i.e. problems installing the dependencies for freesurfer, FSL and AFNI).

Once I ran docker pull with the flag --platform amd64 the build problem disappeared, but performance was miserably slow (which was to be expected, given docker's warning that "image may have poor performance, or fail, if run via emulation").

Similarly, docker pull nipreps/miniconda builds an amd64 build, so I foresee equally slow performance, but I don't really know enough about it to know without testing.

@oesteban oesteban merged commit 9f869eb into master Oct 7, 2021
@oesteban oesteban deleted the docker/multistage-conda branch October 7, 2021 20:42
Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

Forgot to ask this!

name: Pull nipreps/miniconda
command: |
set +e
docker pull localhost:5000/miniconda:1.2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the reasoning for versioning as 1.2.0? Also I think the python version should be included in the tag

Copy link
Member Author

Choose a reason for hiding this comment

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

what's the reasoning for versioning as 1.2.0?

Version history is on the repo, roughly I bumped the minor twice as I added new features. But there's no particular reason, so we could use any suggestions you find appropriate.

Also I think the python version should be included in the tag

Sure, we just need to add it in the tag when creating the release on GitHub.

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.

4 participants