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

Docker purge unneeded files #589

Merged
merged 6 commits into from Jan 21, 2016

Conversation

jakirkham
Copy link
Member

  • Clears the ~/.cache directory at every build step as pip and npm are filling it with junk. ( ~100MB )
  • Clears the ~/.npm directory (just used for cache). ( ~36.7MB )
  • Clears the ~/.config directory after building the notebook (only has bower stuff and bower is removed).
  • Clears the ~/.local directory after building the notebook (only has bower stuff and bower is removed).
  • Clears the ~/tmp directory after building the notebook (is already empty).

@minrk
Copy link
Member

minrk commented Oct 11, 2015

Sheesh, all this tempfile-optimizations really turns the dockerfile into a nightmare.

@jakirkham
Copy link
Member Author

Yeah, it's a bit gross. It is cutting down ~12% of the image's size though. What would you like me to do?

@jakirkham
Copy link
Member Author

Travis failure is unrelated.

@minrk
Copy link
Member

minrk commented Oct 12, 2015

I'm not sure. At this point, I'm starting to lean toward it being a mistake to have added a Dockerfile to this repo in the first place.

@minrk
Copy link
Member

minrk commented Oct 12, 2015

Also: merge conflict, needs rebase.

@minrk
Copy link
Member

minrk commented Oct 12, 2015

To me, the primary value of the Dockerfile in this repo was to provide a basic, illustrative example of installing the notebook. With all of this size-optimization, this is no longer the case, it seems to be optimizing for use as a base image, and I don't think it's a good candidate for a base image, so this seems misguided.

@jakirkham jakirkham force-pushed the docker_purge_unneed_files branch 2 times, most recently from e8aa91d to 96b0c87 Compare October 12, 2015 13:21
@jakirkham
Copy link
Member Author

The merge conflicts have been resolved. The other issues that you have raised are obviously still outstanding.

@minrk minrk added this to the 4.1 milestone Oct 13, 2015
@jakirkham
Copy link
Member Author

FWIW, I do agree that cleaning is a mess.

Normally, this ends up being worse after any sort of testing stage as there really is no interest in keeping such a layer. Instead one is only interested in validating the previously layer works as expected and then continuin/finishing the build without any changes to the final layer caused by testing.

This is certainly not the first time I have seen this issue and felt I should raise it after our discussion here with particular regards to testing. I created an issue here ( moby/moby#16993 ) if you are interested. There are some workarounds proposed relating to local builds and builds on Docker Hub. The real solution will likely rely on some sort of nested build implementation. There are many proposals the most notable being this one ( moby/moby#7115 ). In addition to helping with testing, it seems this proposal might make it easier to avoid clean up at almost any RUN step by doing a nested build and only including the needed build artifacts.

However, as it is very versatile it will take some honing before it makes it into master or a release for that matter. Whether or not the final implementation uses something like dind would determine how versatile this solution would be. Currently, this sort of syntax change is blocked by the freezing of the Dockerfile syntax ( https://github.com/docker/docker/blob/master/ROADMAP.md#22-dockerfile-syntax ). FWICT, it is unclear when the Dockerfile syntax will be unfrozen.

In short, given the interest and people behind this change we can be fairly certain we will see something to alleviate this problem, but given the current blockers it won't be soon.

@Carreau Carreau modified the milestones: 4.x, 4.1 Oct 15, 2015
@minrk minrk modified the milestones: 4.2, 4.x Oct 26, 2015
@minrk minrk changed the title Docker purge unneed files Docker purge unneeded files Jan 21, 2016
minrk added a commit that referenced this pull request Jan 21, 2016
@minrk minrk merged commit 84768ed into jupyter:master Jan 21, 2016
@jakirkham
Copy link
Member Author

So, I guess you changed your mind on this one, @minrk. :)

@jakirkham jakirkham deleted the docker_purge_unneed_files branch January 21, 2016 15:35
@minrk
Copy link
Member

minrk commented Jan 21, 2016

Not really, I just didn't like the PR sitting there, and din't want to make you rebase, since it would conflict with more recent changes.

If we decide this should be a more illustrative example, we will probably remove a lot of the scrubbing in a later PR.

minrk added a commit that referenced this pull request Jan 21, 2016
* Clears the `~/.cache` directory at every build step as `pip` and `npm` are filling it with junk. ( ~100MB )
* Clears the `~/.npm` directory (just used for cache). ( ~36.7MB )
* Clears the `~/.config` directory after building the notebook (only has `bower` stuff and `bower` is removed).
* Clears the `~/.local` directory after building the notebook (only has `bower` stuff and `bower` is removed).
* Clears the `~/tmp` directory after building the notebook (is already empty).
@jakirkham
Copy link
Member Author

Well, I still would like to port the useful changes to the Dockerfile collection at some point, but I'm not sure when that will happen. Did anything ever come out of that discussion about how to manage this and the Dockerfiles going forward?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants