Add docker #236

blezek · 2017-04-05T18:26:50Z

Added two extra Dockerfiles and associated documentation.

A PyRadiomics Docker based on Jupyter notebook Dockers (https://github.com/jupyter/docker-stacks), specifically jupyter/datascience-notebook located here: https://github.com/jupyter/docker-stacks/tree/master/datascience-notebook The Docker exposes the /data volume. The work directory, where the Jupyter notebook starts, has a symlink to /data, making the host directory easily accessable.

…ker PR

* datascience-docker: Rename Dockerfile to Dockerfile.notebook in prep for merging with Docker PR Docker for PyRadiomics based on a Jupyter notebook STYLE: fix syntax ENH: check if feature class name is valid STYL: Remove unused variable BUG: Fix pyradiomicsbatch error when using python 3 BUG: Fix error when image or mask is not loaded correctly

* master: (42 commits) ENH: Small bugfix in commandlinebatch.py DOCS: Document adding a progress reporter ENH: Simplify specifying a progress reporter ENH: Remove dependency for `tqdm` ENH: Add pandas example ENH: Allow variable number of columns in input CSV BUG: Correct Np when weighting is applied STYL: Don't replace , with ; in output of general info BUG: Error in geometry check during resampling STYL: Add FAQ for image and mask of different geometry ENH: Add physical space check when resampling ENH: Add checks for mask size and dimensions. STYL: Update version in README DOCS: Update reference DOCS: Add reference for gray level discretization method STYL: Add suggested value for voxelArrayShift STYL: Add discretization formula MATH: Change default value of voxelArrayShift BUG: Change binning STYL: Rename `force2Dextraction` to `force2D` ...

fedorov

This is great! Just few minor stylistic comments.

fedorov · 2017-04-05T19:38:26Z

Dockerfile.notebook

+
+FROM jupyter/datascience-notebook
+
+MAINTAINER "Daniel Blezek" [email protected]


Probably should be swapped to pyradiomics as in the above, unless you really want to sign up to maintain this personally!

fedorov · 2017-04-05T19:41:18Z

Dockerfile.ubuntu

+
+FROM ubuntu:16.04
+
+MAINTAINER "Daniel Blezek" [email protected]


same as above

fedorov · 2017-04-05T20:18:04Z

Another suggestion is to organize dockerfiles into a dedicated directory

jcfr · 2017-04-05T21:48:12Z

Dockerfile.notebook

+
+# Make a global directory and link it to the work directory
+RUN mkdir /data
+RUN ln -s /data /home/jovyan/work/data


Instead of creating a custom image for this, I would suggest to:

use the official jupyter images

ask the user to simply clone the repository container the notebooks

run the jupyter image mounting that folder

Installation of Pyradiomics and its dependency would be step 1 of the notebook

Doing so, we would have one less image to maintain.

The instructions would then be:

docker pull jupyter/datascience-notebook git clone repo/containing/the-notebooks-dir cd the-notebooks-dir docker run -it --rm -p 8888:8888 -v $(pwd):/home/jovyan/work jupyter/datascience-notebook

jcfr · 2017-04-05T21:54:52Z

README.md

+
+    docker build -t radiomics/cli .
+    docker build -t radiomics/notebook -f Dockerfile.notebook .
+    docker build -t radiomics/cli2 -f Dockerfile.ubuntu .


The image should probably be named differently, it is not clear what is the difference between cli and cli2

jcfr · 2017-04-05T21:56:30Z

Dockerfile.ubuntu

+
+# Install Scipy https://www.scipy.org/install.html
+USER root
+RUN apt-get update && apt-get install -y python-pip python-numpy python-scipy python-nose


Some of these requirements are redundant. Using pip install -r requirements.txt should be sufficient to pull most of the requirements.

jcfr · 2017-04-05T22:00:00Z

Dockerfile

+  python setup.py install
+
+WORKDIR /usr/src
+ENTRYPOINT ["pyradiomics"]


If the goal is to simply have an image providing pyradiomics, executable there is no need to include the compiler in the image to distribute. Instead, I would suggest to build a pyradiomics wheel built using manylinux. These would simply be installed in this image and it would be much smaller.

jcfr

Nice to see docker images coming up for pyradiomics 👍

I would suggest to:

revisit the need for the jupyer based one
consider creating a lighter image to provide pyradiomics. I think there is no need to include the compiler toolchain in the image
consider having pyradiomic wheel based of manylinux that we can there easily integrate in any container
squash all the commits

blezek · 2017-04-06T02:28:44Z

@fedorov, @jcfr, Based on the comments, I'm going to propose that we settle down on 1 Dockerfile. It will be based on the jupyter/datascience-notebook and install the example notebooks, as well as the pyradiomics command. In the documentation, we can give examples on how to start up the notebook interface, and use the command in the Docker image. I do realize the datascience-notebook is heavyweight, but it does come with a huge amount of helpful packages already baked in. Having our examples (and more would be better) already baked in is a huge benefit.

We could also try to produce the smallest possible image for the command line version, if we want to have 2 images.

I hadn't planned on squashing the commits, it didn't seem to be the style in this repo. Is it necessary?

fedorov · 2017-04-06T03:08:17Z

Personally, I really really would hate the perfect to be the enemy of the good here. We can always improve later, but I think it is better to provide this functionality to the user early than have this PR pending much longer.

blezek · 2017-04-06T11:11:42Z

@fedorov Happy to make changes, but I also agree with you. We can iterate on Docker support. I'll let someone else approve the PR.

fedorov · 2017-04-06T11:23:55Z

@blezek would be great if you do those changes that you can and agree with today or tomorrow, we really would like to add docker support before the end of the week in time for the QIN meeting.

pieper · 2017-04-06T11:40:19Z

+1 to making a full featured version available sooner and offering a stripped down version later.

…

On Thu, Apr 6, 2017 at 7:23 AM, Andrey Fedorov ***@***.***> wrote: @blezek <https://github.com/blezek> would be great if you do those changes that you can and agree with today or tomorrow, we really would like to add docker support before the end of the week in time for the QIN meeting. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#236 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHsfXQhlT3g-_J-72OtkDxZsfut3uA0ks5rtMtLgaJpZM4M0ofa> .

jcfr · 2017-04-06T13:21:54Z

offering a stripped down version later.

You are right size of the images is not a critical problem.

That said, the other issues with the current approach is that we do not know if the created images will work or not.

To keep up with the high bar maintained so far 😄 , I would highly recommend to add test(s) checking that the built docker images works as expected. For example:

build and publish the image as part of the CI process (e.g execute a basic notebook that would import pyradiomics and do a simple computation or even run the complete test suite). The new CircleCi 2.0 makes working with docker even easier.

If not we should at least update the release check list to ensure the images are manually tested before each release.

Finally, for any given docker images ... we should be able to tracker what is the corresponding pyradiomics SHA version. This can be done easily adding metadata. See https://microbadger.com/labels#!
For example: https://github.com/QIICR/dcmqi/blob/f621a32eed0eadac852cee9860addbffb5d92460/docker/Makefile#L65-L72

I know it is tempting to move forward .. but I really think having systematic testing and provenance tracking for the docker images will pay off.

pieper · 2017-04-06T13:29:35Z

Absolutely - testing is critical. Let's make sure the user of the image can verify that the image works as we intended. A self test command that the user can easily run would be very helpful. Let's then add running the tests to a notebook tutorial.

…

On Thu, Apr 6, 2017 at 9:21 AM, Jean-Christophe Fillion-Robin < ***@***.***> wrote: offering a stripped down version later. You are right size of the images is now critical problem. That said, the other issues with the current approach is that we do not know if the created images will work or not. To keep up with the high bar maintained so far 😄 , I would hight recommend to add test(s) checking that the built docker images works as expected. For example: - build and publish the image as part of the CI process (e.g execute a basic notebook that would import pyradiomics and do a simple computation or even run the complete test suite). The new CircleCi 2.0 makes working with docker even easier. If not we should at least update the release check list to ensure the images are manually tested before each release. Finally, for any given docker images ... we should be able to tracker what is the corresponding pyradiomics SHA version. This can be done easily adding metadata. See https://microbadger.com/labels#! For example: https://github.com/QIICR/dcmqi/blob/ f621a32eed0eadac852cee9860addbffb5d92460/docker/Makefile#L65-L72 I know it is tempting to move forward .. but I really think having systematic testing and provenance tracking for the docker images will pay off. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#236 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHsfVBfzdx5PZ1ZWrNq5Ugs9v5SxK0yks5rtObzgaJpZM4M0ofa> .

jcfr · 2017-04-06T13:36:06Z

A self test command that the user can easily run would be very helpful. Let's then add running the tests to a notebook tutorial.

Great. To achieve this, we could:

bundle the tests as part of the package. Good news is that it is already the case. See https://github.com/Radiomics/pyradiomics/blob/913473e13fdd9657f5ce9e1c836cd24fac61e748/MANIFEST.in#L14
install and execute nose.
bonus: systematically executing all notebook should also be done.

With that, tests can be executed by anyone installing the package. And in that particular case, since docker provides a consistent and repeatable environment. We can really ensure the tests run in the environment we provide right after we build the docker images.

pieper · 2017-04-06T13:41:40Z

Sounds awesome!

…

On Thu, Apr 6, 2017 at 9:36 AM, Jean-Christophe Fillion-Robin < ***@***.***> wrote: A self test command that the user can easily run would be very helpful. Let's then add running the tests to a notebook tutorial. Great. To achieve this, we could: - bundle the tests as part of the package. Good news is that it is already the case. See https://github.com/Radiomics/pyradiomics/blob/ 913473e/MANIFEST.in#L14 <https://github.com/Radiomics/pyradiomics/blob/913473e13fdd9657f5ce9e1c836cd24fac61e748/MANIFEST.in#L14> - install and execute nose. - bonus: systematically executing all notebook should also be done. With that, tests can be executed by anyone installing the package. And in that particular case, since docker provides a consistent and repeatable environment. We can really ensure the tests run in the environment we provide right after we build the docker images. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#236 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHsfQDB9GSF9E52uOdu6coiqejrN30lks5rtOpGgaJpZM4M0ofa> .

jcfr · 2017-04-06T13:51:34Z

Great.

@blezek Let us know if you have any questions. I would be happy to review further.

Keep getting errors such as: `Directory (/) you are trying to checkout to is not empty and not git repository` Apparently, have to specify an empty directory for CircleCI.

blezek · 2017-04-07T20:09:09Z

Hey @jcfr @pieper @fedorov & @JoostJM,

Bunch of commits, but I slimmed down to one Dockerfile that runs the example notebooks as part of the build with Python 2 and 3. Also added a CircleCI v2 configuration that installs radiomics under Python 2 & 3, test the package and executes the notebooks.

A working example build is at https://circleci.com/gh/blezek/pyradiomics/9.

Added labels to the Docker (https://github.com/rossf7/label-schema-automated-build) but can't figure out how to test it without going through DockerHub.

If we configure DockerHub to auto build, we should have a nice pipeline going.

pieper · 2017-04-07T20:28:04Z

Hi @blezek I checked out the branch and ran 'docker build' but got the error below.

---> ebf7a1a7a6a3
Removing intermediate container 66d0c72f5b2d
Step 13 : ADD bin/Notebooks/FeatureVisualizationWithClustering.ipynb /home/jovyan/work/
lstat bin/Notebooks/FeatureVisualizationWithClustering.ipynb: no such file or directory

The file is referenced here, but looks like it's not in your branch.

https://github.com/Radiomics/pyradiomics/pull/236/files#diff-3254677a7917c6c01f55212f86c57fbfR33

blezek · 2017-04-07T20:29:20Z

Sorry @pieper, I missed it. Try again. Need someone to look at that file and decide if it should be added to the Docker or not.

fedorov · 2017-04-07T23:39:06Z

I confirm I could build docker without problems. Thank you Dan!

fedorov · 2017-04-09T12:27:51Z

I am merging this so we officially have docker support in time for the QIN meeting that starts tomorrow.

fedorov · 2017-04-09T12:56:14Z

@JoostJM you will need to configure docker hub yourself, it does not work for me, perhaps because of some permission issue. You will need to create Automated build, not Repository, which you have at the moment. I cannot do this, because somehow radiomics organization is not showing up in the list when I initiate creation of a new Automated build.

fedorov · 2017-04-10T01:54:33Z

I was not able to access radiomics/pyradiomics organization from Docker hub because of the third-party access restriction policy. After disabling it, I can see the organization and the repo. However, I am still not able to create automated build under radiomics organization on Docker hub.

JoostJM · 2017-04-10T01:56:37Z

@fedorov, I just added it. Setting it up now.

JoostJM · 2017-04-10T01:58:48Z

PyRadiomics docker is building.

JoostJM · 2017-04-10T18:44:57Z

bin/Notebooks/PyRadiomics Example.ipynb

@@ -1,481 +0,0 @@
-{


@blezek, you deleted this example, was there a specific reason for this? This example was also used in the short video we made on PyRadiomics usage, and was therefore also made available in the repository

Oops, @JoostJM I'm terribly sorry, I mistakenly deleted this notebook! I thought it was one of my mistakes. I'll submit a PR and fix it.

fedorov and others added 17 commits February 23, 2017 09:23

ENH: add initial Dockerfile

9de1169

ENH: fix maintainer

fd1c44b

Rename Dockerfile to Dockerfile.notebook in prep for merging with Doc…

4b62da5

…ker PR

Updated README.md to reflect two Dockers

e70873e

Removed unneeded Docker build steps

9c8bd0b

Add Ubuntu Docker and updated documentation

411eb1d

Set verbose to False

65bd7a5

Notebook includes two new examples, hushed cache warning from pip

ab1b66e

Removed verbosity setting, it's unused

18dfed3

Documentation changes

a951b8e

Change ownership of working directory and trust notebooks

f7d5305

Adds heatmap example

f8fe6c5

Updated example Notebooks

e9fc87a

Changed clustermap plotting to show feature names horizontally on y axis

2aecfb0

blezek requested review from fedorov and JoostJM April 5, 2017 18:26

fedorov approved these changes Apr 5, 2017

View reviewed changes

jcfr reviewed Apr 5, 2017

View reviewed changes

jcfr requested changes Apr 5, 2017

View reviewed changes

blezek added 8 commits April 7, 2017 11:39

Migrating Dockerfiles

1d76e30

Migrating to CircleCI 2

672e7b2

Proper syntax on CircleCI v2 build file

2a87b4c

Install SSH in docker for testing

b09910f

Tests by building against the base Docker

368cb38

Don't set the working directory

7b4c812

Have to set the working directory after all...

2419c69

Specify a nonexistent directory as a working directory

f81430a

Keep getting errors such as: `Directory (/) you are trying to checkout to is not empty and not git repository` Apparently, have to specify an empty directory for CircleCI.

Add missing clustering file

c134e38

fedorov merged commit d151d44 into AIM-Harvard:master Apr 9, 2017

JoostJM reviewed Apr 10, 2017

View reviewed changes

blezek mentioned this pull request Apr 11, 2017

Restore accidentally removed file #238

Merged

JoostJM added the enhancement label May 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add docker #236

Add docker #236

blezek commented Apr 5, 2017

fedorov left a comment

fedorov Apr 5, 2017

fedorov Apr 5, 2017

fedorov commented Apr 5, 2017

jcfr Apr 5, 2017

jcfr Apr 5, 2017 •

edited

Loading

jcfr Apr 5, 2017

jcfr Apr 5, 2017 •

edited

Loading

jcfr Apr 5, 2017

jcfr left a comment •

edited

Loading

blezek commented Apr 6, 2017 •

edited

Loading

fedorov commented Apr 6, 2017

blezek commented Apr 6, 2017

fedorov commented Apr 6, 2017

pieper commented Apr 6, 2017 via email

jcfr commented Apr 6, 2017 •

edited

Loading

pieper commented Apr 6, 2017 via email

jcfr commented Apr 6, 2017

pieper commented Apr 6, 2017 via email

jcfr commented Apr 6, 2017

blezek commented Apr 7, 2017 •

edited

Loading

pieper commented Apr 7, 2017

blezek commented Apr 7, 2017

fedorov commented Apr 7, 2017

fedorov commented Apr 9, 2017

fedorov commented Apr 9, 2017

fedorov commented Apr 10, 2017

JoostJM commented Apr 10, 2017

JoostJM commented Apr 10, 2017

JoostJM Apr 10, 2017

blezek Apr 11, 2017


		FROM jupyter/datascience-notebook

		MAINTAINER "Daniel Blezek" [email protected]


		FROM ubuntu:16.04

		MAINTAINER "Daniel Blezek" [email protected]

Add docker #236

Add docker #236

Conversation

blezek commented Apr 5, 2017

fedorov left a comment

Choose a reason for hiding this comment

fedorov Apr 5, 2017

Choose a reason for hiding this comment

fedorov Apr 5, 2017

Choose a reason for hiding this comment

fedorov commented Apr 5, 2017

jcfr Apr 5, 2017

Choose a reason for hiding this comment

jcfr Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

jcfr Apr 5, 2017

Choose a reason for hiding this comment

jcfr Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

jcfr Apr 5, 2017

Choose a reason for hiding this comment

jcfr left a comment • edited Loading

Choose a reason for hiding this comment

blezek commented Apr 6, 2017 • edited Loading

fedorov commented Apr 6, 2017

blezek commented Apr 6, 2017

fedorov commented Apr 6, 2017

pieper commented Apr 6, 2017 via email

jcfr commented Apr 6, 2017 • edited Loading

pieper commented Apr 6, 2017 via email

jcfr commented Apr 6, 2017

pieper commented Apr 6, 2017 via email

jcfr commented Apr 6, 2017

blezek commented Apr 7, 2017 • edited Loading

pieper commented Apr 7, 2017

blezek commented Apr 7, 2017

fedorov commented Apr 7, 2017

fedorov commented Apr 9, 2017

fedorov commented Apr 9, 2017

fedorov commented Apr 10, 2017

JoostJM commented Apr 10, 2017

JoostJM commented Apr 10, 2017

JoostJM Apr 10, 2017

Choose a reason for hiding this comment

blezek Apr 11, 2017

Choose a reason for hiding this comment

jcfr Apr 5, 2017 •

edited

Loading

jcfr Apr 5, 2017 •

edited

Loading

jcfr left a comment •

edited

Loading

blezek commented Apr 6, 2017 •

edited

Loading

jcfr commented Apr 6, 2017 •

edited

Loading

blezek commented Apr 7, 2017 •

edited

Loading