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

clean up Dockerfile #83

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

paketb0te
Copy link
Contributor

I saw that the Dockerfile installs git-sim from pip (RUN pip3 install git-sim), I think it makes more sense to copy the code and install it directly - hence this PR :)

While at it, I also removed some packages that are not required (python3-dev, and setting --no-install-recommends).

The syntax for calling git-sim in Docker does not change (-> no changes to README.md).

@initialcommit-io
Copy link
Contributor

@paketb0te I'm inclined to leave it as is right now since it seems to be working (haven't had users report any issues with the docker install in a while).

Can you let me know the benefits of changing the current setup, including:

  • difference between the default python3 image and python3.10-bullseye
  • in manim installation page for linux it does say that python3-dev is a requirement. Is this already included in the base python image?
  • other pros/cons between the 2 setups

Thx!

@initialcommit-io
Copy link
Contributor

@paketb0te Oh one other thing - if we are considering changing to specific Python version base image, why not use a 3.11 image to take advantage of the performance updates?

@paketb0te
Copy link
Contributor Author

paketb0te commented Apr 11, 2023

* difference between the default python3 image and python3.10-bullseye

No difference, I just prefer using a specific version that will not change as much - using the tag python:3, you can not know if that image points to the 3.9.,3.10, 3.11, 3.12 version... (usually the latest stable, so probably 3.11 at the moment?)

* in [manim installation page for linux](https://docs.manim.community/en/stable/installation/linux.html) it does say that `python3-dev` is a requirement. Is this already included in the base python image?

Yes, the image has python installed already (kinda obvious, it's a python image after all :D ).

From my testing, the -dev version is not needed, and it helps to avoid having a duplicate python installation in the image.

* other pros/cons between the 2 setups

I think it makes more sense to build the image from source, instead of installing from pypi - we can a) build in parallel, if at some point we decide to publish the image to dockerhub (or any other registry), and b) it makes testing locally with docker possible when developing, (since we get the current source code in the container, not whatever is the latest version on pypi).

Agreed, these are all rather small reasons and I have no problems leaving things as-is, just thought it might be an easy, small improvement :)

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.

2 participants