-
Notifications
You must be signed in to change notification settings - Fork 26
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
Simplify and optimize Docker image #264
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
@edoardob90 do you need a review on this? I could give a hand. |
If you want, you can try to build the container from the Dockerfile. I tested the workflow in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @edoardob90, I did a first pass and I would need some clarifications from your side.
|
||
# Copy the repository late in the build process | ||
RUN mkdir -p ${REPO} | ||
COPY --chown=${NB_UID}:${NB_GID} . ${REPO}/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is a bit tricky. The user should either start the same container (so the readme needs to be updated), or we should mount the folder to the container.
In any case, the readme should be synchronized with the new approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the step is correct and makes sense only when we're building the image via GitHub Actions. The user can still mount the local folder with a local copy of the repository, and nothing changes if the are no mismatches. Otherwise they will have their local repository inside the container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to create a docker volume, but I have to think about it carefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a bit of research. It seems that those lines are not needed: I added them to make sure to have the repository material inside the container, but:
- If the user is mounting a local folder with the repository, that's redundant
- GitHub Codespaces, which can use a prebuilt Docker image to speed up the startup, also clones the repository by default, so the lines are again redundant
If you agree that this is the case, we can remove them and speed up building the image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we have a quick chat about this today during the meeting, and then we can decide.
707acaf
to
3786be6
Compare
Options: Leave the
|
jupyter-repo2docker
dependencydocker/environment.yml
The old workflow has been renamed to
.github/workflows/repo2docker.yml
and Binder is still supported via thebinder/
directory (unchanged).Also: since Docker provides already the isolation, there's no need to have another virtual environment. All the tutorial dependencies are installed in the
base
environment, which is automatically configured by the base image.