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

add Dockerfile #522

Merged
34 commits merged into from Apr 4, 2017
Merged

add Dockerfile #522

34 commits merged into from Apr 4, 2017

Conversation

ghost
Copy link

@ghost ghost commented Apr 3, 2017

for people that use docker, they will easily be able to install moviepy.

@coveralls
Copy link

coveralls commented Apr 3, 2017

Coverage Status

Coverage remained the same at 48.883% when pulling 17d882d on earney:docker into c611107 on Zulko:master.

@tburrows13
Copy link
Collaborator

I assume this is intended to replace #65?

Copy link
Contributor

@kencochrane kencochrane left a comment

Choose a reason for hiding this comment

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

Looks good so far, left a few comments.

Dockerfile Outdated

# install from github since there are bugs in latest versions.
RUN git clone https://github.com/Zulko/moviepy.git /var/src/moviepy
RUN cd /var/src/moviepy/ && python setup.py install
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the Dockerfile is going to be included with the source, we won't need to pull down from git, we can just include it like this.

ADD . /var/src/moviepy/
RUN cd /var/src/moviepy/ && python setup.py install

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this comment might get lost. Commenting again in the thread, incase it does.

Dockerfile Outdated
# install scikit-image after the other deps, it doesn't cause errors this way.
RUN pip install scikit-image sklearn

# install from github since there are bugs in latest versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove this comment now.

Dockerfile Outdated
RUN cd /var/src/moviepy/ && python setup.py install

# install ffmpeg from imageio.
RUN python -c "import imageio; imageio.plugins.ffmpeg.download()"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will install ffmpeg into a weird location not in the path, should we add a symlink or something to put in in a location that is more common?

Copy link
Author

Choose a reason for hiding this comment

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

sure.. I'll work on adding that.. Thanks for pointing this out.

@ghost
Copy link
Author

ghost commented Apr 3, 2017

@Gloin1313 , yes this replaces #65. The dockerfile in #65 doesn't work anymore, and the author has not responded, or participated in the discussion, so I assume he is AWOL.

@coveralls
Copy link

coveralls commented Apr 3, 2017

Coverage Status

Coverage remained the same at 48.883% when pulling 1310c83 on earney:docker into c611107 on Zulko:master.

Dockerfile Outdated
RUN pip install scikit-image sklearn

RUN git clone https://github.com/Zulko/moviepy.git /var/src/moviepy
RUN cd /var/src/moviepy/ && python setup.py install
Copy link
Contributor

Choose a reason for hiding this comment

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

Re-commenting since it is no longer showing up.

Since the Dockerfile is going to be included with the source, we won't need to pull down from git, we can just include it like this.

ADD . /var/src/moviepy/
RUN cd /var/src/moviepy/ && python setup.py install

@coveralls
Copy link

coveralls commented Apr 3, 2017

Coverage Status

Coverage remained the same at 48.883% when pulling 1c16e6d on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 48.883% when pulling 9317774 on earney:docker into c611107 on Zulko:master.

1 similar comment
@coveralls
Copy link

coveralls commented Apr 3, 2017

Coverage Status

Coverage remained the same at 48.883% when pulling 9317774 on earney:docker into c611107 on Zulko:master.

@kencochrane
Copy link
Contributor

👍 LGTM, thanks for putting this together.

@ghost
Copy link
Author

ghost commented Apr 3, 2017

We will want to write some documentation on how to use the dockerfile. Are there standard documentation standards that docker uses, that we could adopt so that people fluent in docker can hit the ground running?

@kencochrane
Copy link
Contributor

@Earney good idea. Take a look at this gist, and let me know if that seems reasonable. https://gist.github.com/kencochrane/9047c8765fc127449b7760726f113cda It is for a dockerfile for building the moviepy docs, but the directions should be similar.

It basically breaks down to these steps

  1. get docker and install it
  2. show people where the Dockerfile is located and how to build it
  3. show people how to run it, and common use cases.

Once this PR is merged, we can add an automated build on docker hub, which will remove the need for people to build the docker image themselves, they will be able to just pull the moviepy image from the hub, and get started.

@coveralls
Copy link

coveralls commented Apr 4, 2017

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

@coveralls
Copy link

coveralls commented Apr 4, 2017

Coverage Status

Coverage increased (+0.07%) to 48.954% when pulling b085e16 on earney:docker into c611107 on Zulko:master.

Copy link
Contributor

@kencochrane kencochrane left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

@ghost ghost merged commit 2a71175 into Zulko:master Apr 4, 2017
@ghost
Copy link
Author

ghost commented Apr 5, 2017

@kencochrane will docker automatically create a moviepy image since the Dockerfile has been created? When the image is created, how do people find it, and use it? I'd like to add some of these details to the docker rst file I created earlier this week. Thanks!

@ghost ghost deleted the docker branch April 5, 2017 12:41
@kencochrane
Copy link
Contributor

@Earney we will need to setup an automated build, which tells Docker Hub where to find the Dockerfile, and sets up a github webhook to let hub know when there are changes to the git repo, so it can rebuild.

I have claimed the moviepy organization on docker hub. https://hub.docker.com/u/moviepy/ If you give me your Docker ID, I can give you access to the organization, so that you can create the automated build. (once I hand it over to you, we can remove me from the organization as well, so I don't have access to do anything I shouldn't be allowed to do)

I recommend that we setup the automated build like this.

  • moviepy/moviepy:lastest is tied to Master, whenever a new commit is pushed to master it will rebuild, and always be in sync with github master branch
  • moviepy/moviepy:tag where the docker tag is the same as the moviepy git tag. This will allow people to pull down specific versions of moviepy, if they don't want to work with the bleeding edge. This is created automatically when ever a new github tag is created.

Once we have the automated builds setup, we can add some directions to the docker.rst that explain how to pull the files from docker hub, vs having to build the images themselves.

@ghost
Copy link
Author

ghost commented Apr 5, 2017

My docker id is earney

@kencochrane
Copy link
Contributor

@Earney thanks, just invited you to the org. If you need help with anything, let me know.

@ghost
Copy link
Author

ghost commented Apr 5, 2017

Thanks.. I'll take a look.

@ghost
Copy link
Author

ghost commented Apr 5, 2017

@kencochrane when I try to configure the autobuilds, zulko is not in the list of github organization I can choose from (but earney is).

@kencochrane
Copy link
Contributor

@Earney I think Zulko is a personal account on github, and not an organization, so you are probably just a collaborator on that repo, and github doesn't give you access to administrator that repo, like it would if it was an organization. zulko, will probably be the one that needs to setup the automated build since it is tied to his github account, and not an organization.

I also noticed that you made a repo called moviepy/latest, we should probably make the repo named moviepy/moviepy and then use tagging to handle latest, this way we have one repo, with different tags, vs having a different repo for each tag, which becomes a maintenance nightmare.

@ghost
Copy link
Author

ghost commented Apr 5, 2017

@kencochrane I was afraid of that..

I couldn't make moviepy/moviepy:latest (it complained about the :).. I'll remove that repo, and will create one called moviepy/moviepy.

@kencochrane
Copy link
Contributor

@Earney ok, sounds good. thank you

@mbeacom
Copy link
Collaborator

mbeacom commented Apr 5, 2017

We might benefit from consolidating the image layers to something like:

FROM python:3-slim

ENV LC_ALL C.UTF-8

ADD . /var/src/moviepy/

RUN apt-get -y update && \
    apt-get -y install libav-tools imagemagick libopencv-dev python-opencv fonts-liberation locales && \
    locale-gen C.UTF-8 && \
    /usr/sbin/update-locale LANG=${LC_ALL} && \
    pip install imageio numpy scipy matplotlib pandas decorator tqdm pillow scikit-image nose pytest sklearn && \
    cd /var/src/moviepy/ && \
    python setup.py install && \
    python -c "import imageio; imageio.plugins.ffmpeg.download()" && \
    ln -s /root/.imageio/ffmpeg/ffmpeg.linux64 /usr/bin/ffmpeg && \
    cat /etc/ImageMagick-6/policy.xml | sed 's/none/read,write/g'> /etc/ImageMagick-6/policy.xml && \
    rm -rf /var/lib/apt/lists/*

Ideally, we'd want to make this image smaller (while decreasing the number of layers we introduce) for easier distribution and usage. We'd probably see the largest decrease in image size from rolling our own image directly versus using the python image. During my testing of size, I ended up splitting up the requirements into base/test/doc.txt files inheriting from base.txt. We can probably reevaluate the apt packages being installed as well.

Additionally, we might want to consider switching off of the liberation font

👍 for an official org on docker hub.

Thanks for working on this @kencochrane @Earney !

@kencochrane
Copy link
Contributor

@mbeacom I agree we should limit the number of layers, but unfortunately your approach above, would cause other problems. This would cause one RUN command and one layer, but if anything changes in that RUN it will break the build cache, specifically the python setup.py install would break the cache and the whole command would need to be run every single time, and we would lose build caching. When you lose your build cache, the layer needs to be rebuilt, and it will make the builds take much longer.

Using your approach, we can probably do something in the middle, instead of merging to just one RUN command, break it down into a few.

  • system packages
  • pip packages
  • moviepy install
  • system config stuff

There is a new feature in recent versions of docker that let you squash your layers after build, and keep your cache, so the end result is the same, but you get caching, which removes the need to build each layer every time. https://docs.docker.com/engine/reference/commandline/build/#squash-an-images-layers---squash-experimental-only

If we want smaller packaging we can look at using an alpine based image (python:3-alpine), but switching to alpine brings in other challenges, like finding the right system libraries etc.

Switching to python:3-slim is another good idea, if it works for our use case.

What ever we do, we should probably stay on top of an official image, this way we will automatically get security and bug fixes from upstream and one less thing to have to manage outside of the scope of this project.

@mbeacom
Copy link
Collaborator

mbeacom commented Apr 5, 2017

@kencochrane Regarding build cache, you're right.

Maybe it's bad practice on my end, but when distributing images for general usage, I try to drop additional layers.

Normally I use alpine-based images whenever possible. 👍 For alpine! Sadly from the look of it, we'll run into issues with locales (see: gliderlabs/docker-alpine#144). Among the many other oddities we'll run into.

@kencochrane
Copy link
Contributor

@mbeacom yeah, alpine is great, but since it isn't as popular there are some edge cases that normally cause issues. I'm fine with rolling our own image based off an official base image (debian, ubuntu, etc) that isn't a python base image, just as long as we aren't doing everything from scratch :)

@ghost
Copy link
Author

ghost commented Apr 5, 2017

I believe a python:3-slim is a good compromise. I can test this out and see if it works on the test cases, if so we might want to start out with it, and adjust when necessary.

@tburrows13 tburrows13 added the feature New addition to the API i.e. a new class, method or parameter. label Apr 17, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition to the API i.e. a new class, method or parameter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants