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

Docker refresh #5153

Merged
merged 1 commit into from
Jan 20, 2017
Merged

Docker refresh #5153

merged 1 commit into from
Jan 20, 2017

Conversation

cypof
Copy link
Member

@cypof cypof commented Jan 4, 2017

I'd like to have an official image for Caffe, so updated the builds to the latest Ubuntu, CUDA and cuDNN, and added NCCL. Also I'm not sure I see the point of maintaining the docker Makefile and templates now that images build automatically, so I tentatively removed them.

@shelhamer shelhamer added the focus label Jan 4, 2017
# FIXME: clone a specific git tag and use ARG instead of ENV once DockerHub supports this.
ENV CLONE_TAG=master
# FIXME: use ARG instead of ENV once DockerHub supports this
ENV CLONE_TAG=1.0.0-rc4
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not a valid bvlc/caffe tag, right?

@@ -1,4 +1,4 @@
FROM ubuntu:14.04
FROM ubuntu:16.04
MAINTAINER [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

MAINTAINER is being deprecated in Docker 1.13, you should use a label instead, might as well change it now.
You can use something like this instead.

# FIXME: clone a specific git tag and use ARG instead of ENV once DockerHub supports this.
ENV CLONE_TAG=master
# FIXME: use ARG instead of ENV once DockerHub supports this
ENV CLONE_TAG=1.0.0-rc4
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -1,4 +1,4 @@
FROM nvidia/cuda:7.5-cudnn5-devel-ubuntu14.04
FROM nvidia/cuda:8.0-cudnn5-devel-ubuntu16.04
MAINTAINER [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

for req in $(cat python/requirements.txt) pydot; do pip install $req; done && \
pip install --upgrade pip && \
cd python && for req in $(cat requirements.txt) pydot; do pip install $req; done && cd .. && \
git clone https://github.com/NVIDIA/nccl.git && cd nccl && make -j install && cd .. && \
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably remove the NCCL directory after install, to save space.

@cypof
Copy link
Member Author

cypof commented Jan 6, 2017

Thanks @flx42 should be good now

@flx42
Copy link
Contributor

flx42 commented Jan 6, 2017

@cypof what about the git clone tag?

@cypof
Copy link
Member Author

cypof commented Jan 6, 2017

I pushed a tag, it should be there

@flx42
Copy link
Contributor

flx42 commented Jan 6, 2017

Great, LGTM then.

@flx42
Copy link
Contributor

flx42 commented Jan 6, 2017

Actually, not LGTM. -DUSE_NCCL=1 is not supported in current BVLC/caffe. I take it this PR comes after #4563?

@cypof
Copy link
Member Author

cypof commented Jan 6, 2017

Ah yes, I was assuming nccl would make it. I just refreshed #4563 history so it should be ready now. Let's wait until it goes in.

@flx42
Copy link
Contributor

flx42 commented Jan 17, 2017

Now that #4563 was merged, you should create a new tag (rc5?) for your Dockerfile.

@shelhamer
Copy link
Member

Dropping the Makefile seems fine as long as the docs clearly explain how to build the images yourself for those who want to work with their own branches.

@cypof
Copy link
Member Author

cypof commented Jan 18, 2017

Yes this PR improves the readme too. It's simpler to get started and it shows how to build the images. I think this is good to go, I will make sure to update the tag as soon as we have an official rc4.

@cypof
Copy link
Member Author

cypof commented Jan 20, 2017

Pointing to rc4 now.

@flx42
Copy link
Contributor

flx42 commented Jan 20, 2017

LGTM

@shelhamer
Copy link
Member

@cypof feel free to merge when ready

@cypof cypof merged commit b8fa34d into BVLC:master Jan 20, 2017
aim-endo pushed a commit to ideeinc/caffe that referenced this pull request Feb 6, 2017
Docker refresh: simplified & update to 16.04, cuda8, cudnn5, nccl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants