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

Refactor Dockerfiles to use common base layer #92

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor Dockerfiles to use common base layer #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37b2f00
Refactor Dockerfiles to use common base layer
benbarsdell Jun 10, 2017
8b5bf91
Fix travis build by setting docker tag :latest-cpu
benbarsdell Jun 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ install:
- make docker-cpu

script:
- docker run --rm ledatelescope/bifrost /bin/sh -c "cd /bifrost/test && sh ./travis.sh"
- docker run --rm ledatelescope/bifrost:latest-cpu /bin/sh -c "cd /bifrost/test && sh ./travis.sh"
- bash ./.travis_deploy_docs.sh

env:
Expand Down
19 changes: 19 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Note: FROM command is inserted by Makefile
# TODO: As of Docker-CE 17.05, we could use this better approach instead:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can also build from streams (moby/moby#31236) which is nice, so we won't have to create a temporary file within the build context.

#ARG base_layer
#FROM $base_layer

MAINTAINER Ben Barsdell <benbarsdell@gmail.com>

ARG make_args

# Build the library
WORKDIR /bifrost
COPY . .
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should create a .dockerignore file with at least .git included.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find it can be useful to have the git history inside the container, but perhaps I should be doing that with your approach of using the base container and mapping the source directory instead. In that case, this sounds like a good idea.

RUN make clean && \
make -j16 $make_args && \
make doc && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’ve heard the best docker practice is to make containers
as minimal as possible (?), so I don't think the full doxygen reference should go in. It would be hard to visualize it efficiently without setting up x11 as well, and we have a website to hold it all anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, this make clean is probably causing the version.py error. Does that need to be in there? Is it there just in case the user already built bifrost outside the container?

Copy link
Collaborator Author

@benbarsdell benbarsdell Jun 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Removing the make doc sounds like the right idea.

Indeed the make clean is important to avoid strange errors when the user already built the library outside. I'll look into what's going on with version.py.

We could probably also do make clean again after make install.

make install

WORKDIR /workspace
RUN ["/bin/bash"]
11 changes: 4 additions & 7 deletions Dockerfile.cpu → Dockerfile.base
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
FROM ubuntu:14.04
# Note: FROM command is inserted by Makefile
# TODO: As of Docker-CE 17.05, we could use this better approach instead:
#ARG base_layer
#FROM $base_layer

MAINTAINER Ben Barsdell <benbarsdell@gmail.com>

Expand Down Expand Up @@ -41,16 +44,10 @@ ENV TERM xterm

# Build the library
WORKDIR /bifrost
COPY . .
RUN make clean && \
make -j NOCUDA=1 && \
make doc && \
make install

ENV LD_LIBRARY_PATH /usr/local/lib:${LD_LIBRARY_PATH}

# IPython
EXPOSE 8888

WORKDIR /workspace
RUN ["/bin/bash"]
56 changes: 0 additions & 56 deletions Dockerfile.gpu
View file
Open in desktop

This file was deleted.

50 changes: 0 additions & 50 deletions Dockerfile_prereq.gpu
View file
Open in desktop

This file was deleted.

39 changes: 25 additions & 14 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,23 +47,34 @@ python: libbifrost
.PHONY: python

#GPU Docker build
IMAGE_NAME ?= ledatelescope/bifrost
docker:
docker build --pull -t $(IMAGE_NAME):$(LIBBIFROST_MAJOR).$(LIBBIFROST_MINOR) -f Dockerfile.gpu -t $(IMAGE_NAME) .
docker-base:
echo "FROM $(CUDA_IMAGE_NAME)" > _Dockerfile.tmp
Copy link
Collaborator

Choose a reason for hiding this comment

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

These could all be .dockerfile.tmp, instead of _dockerfile.tmp, the latter of which seems like a python-specific convention for private/hidden objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SGTM

cat Dockerfile.base >> _Dockerfile.tmp
docker build --pull -t $(IMAGE_NAME):latest-base -t $(IMAGE_NAME):$(LIBBIFROST_MAJOR).$(LIBBIFROST_MINOR)-base -f _Dockerfile.tmp .
rm _Dockerfile.tmp
.PHONY: docker-base

docker: docker-base
echo "FROM $(IMAGE_NAME):latest-base" > _Dockerfile.tmp
cat Dockerfile >> _Dockerfile.tmp
docker build --build-arg make_args="" -t $(IMAGE_NAME):latest -t $(IMAGE_NAME):$(LIBBIFROST_MAJOR).$(LIBBIFROST_MINOR) -f _Dockerfile.tmp .
rm _Dockerfile.tmp
.PHONY: docker

#GPU Docker prereq build
# (To be used for testing new builds rapidly)
IMAGE_NAME ?= ledatelescope/bifrost
docker_prereq:
docker build --pull -t $(IMAGE_NAME)_prereq:$(LIBBIFROST_MAJOR).$(LIBBIFROST_MINOR) -f Dockerfile_prereq.gpu -t $(IMAGE_NAME)_prereq .
.PHONY: docker_prereq

#CPU-only Docker build
IMAGE_NAME ?= ledatelescope/bifrost
docker-cpu:
docker build --pull -t $(IMAGE_NAME):$(LIBBIFROST_MAJOR).$(LIBBIFROST_MINOR) -f Dockerfile.cpu -t $(IMAGE_NAME) .
.PHONY: docker
docker-base-cpu:
echo "FROM $(CPU_IMAGE_NAME)" > _Dockerfile.tmp
cat Dockerfile.base >> _Dockerfile.tmp
docker build --pull -t $(IMAGE_NAME):latest-base-cpu -t $(IMAGE_NAME):$(LIBBIFROST_MAJOR).$(LIBBIFROST_MINOR)-base-cpu -f _Dockerfile.tmp .
rm _Dockerfile.tmp
.PHONY: docker-base

docker-cpu: docker-base-cpu
echo "FROM $(IMAGE_NAME):latest-base-cpu" > _Dockerfile.tmp
cat Dockerfile >> _Dockerfile.tmp
docker build --build-arg make_args="NOCUDA=1" -t $(IMAGE_NAME):latest-cpu -t $(IMAGE_NAME):$(LIBBIFROST_MAJOR).$(LIBBIFROST_MINOR)-cpu -f _Dockerfile.tmp .
rm _Dockerfile.tmp
.PHONY: docker-cpu

# TODO: Consider adding a mode 'develop=1' that makes symlinks instead of copying
# the library and headers.
Expand Down
5 changes: 5 additions & 0 deletions user.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ CUDA_INCDIR ?= $(CUDA_HOME)/include

ALIGNMENT ?= 4096 # Memory allocation alignment

# Docker image names
IMAGE_NAME ?= ledatelescope/bifrost
CUDA_IMAGE_NAME ?= nvidia/cuda:8.0 # Base layer for the GPU (default) images
CPU_IMAGE_NAME ?= ubuntu:16.04 # Base layer for the -cpu images

#NODEBUG = 1 # Disable debugging mode (use this for production releases)
#TRACE = 1 # Enable tracing mode (generates annotations for use with nvprof/nvvp)
#NOCUDA = 1 # Disable CUDA support
Expand Down