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

Optimised Dockerfile for fast, cached builds #292

Merged
merged 5 commits into from
May 12, 2017
Merged

Optimised Dockerfile for fast, cached builds #292

merged 5 commits into from
May 12, 2017

Conversation

angadn
Copy link
Contributor

@angadn angadn commented Apr 20, 2017

No description provided.

@tmbo tmbo requested a review from twerkmeister April 20, 2017 12:17
@twerkmeister
Copy link
Contributor

twerkmeister commented Apr 25, 2017

@angadn yet another contribution, thanks a lot! Good idea with the caching. Can you give me a short explanation why you remove -e . from the requirements.txt just to add it later again?

@angadn
Copy link
Contributor Author

angadn commented Apr 25, 2017

@twerkmeister the idea is to first install all dependencies without copying in our own code-base inside the container, which we'd have to do if we were installing our modules alongside; since Docker would then watch our files for change, and install all dependencies afresh. In this case, you'll notice that if you try to move the COPY instruction to after the requirements-setup, it will throw an error saying it couldn't find setup.py in the current directory (copy that file in isolation, and errors start cascading all the way to inside our folder-structure).

On line 26, I concatenate the -e . ahead of the requirements, and ask for install again - this time, all the cached dependencies are already found, and just our modules are installed!

@twerkmeister
Copy link
Contributor

Thanks for the explanation! What happens now if someone executes setup.py outside of the docker build file? Then the -e is missing unless they add it manually. Wouldn't it be better if: 1) we keep the -e in requirements.txt, 2) copy requirements.txt it into the container, 3) create a requirements-nonlocal.txt by cutting of the -e with something like cat requirements.txt | grep -v "^-e .$" > requirements-nonlocal.txt and 4) execute pip install -r requirements-nonlocal.txt. This way the non-docker build processes wouldn't be affected at all

@angadn
Copy link
Contributor Author

angadn commented Apr 30, 2017

Crap - to be honest, this is the first time I'm working on a Python codebase in months; makes sense, I shall make this change and update the PR as soon as I'm back at work!

@tmbo
Copy link
Member

tmbo commented May 8, 2017

@angadn please let us know when you have an updated version, this is quite a neat contribution 👍

…on, so as to preserve functionality of non-docker setups (as per: #292 (comment))
@angadn
Copy link
Contributor Author

angadn commented May 8, 2017

Pushed update as per suggestions - testing out the build process locally now and will push fixes if necessitated

@angadn
Copy link
Contributor Author

angadn commented May 9, 2017

Not sure why 1 check is failing - can you please point me in the right direction

@tmbo
Copy link
Member

tmbo commented May 9, 2017

@angadn that issue is not related to your code, merging in the latest master should resolve it.

@angadn
Copy link
Contributor Author

angadn commented May 9, 2017

Whoops - done. Hope this helps

@tmbo
Copy link
Member

tmbo commented May 10, 2017

@twerkmeister please merge if you feel everything is good to go.

@twerkmeister
Copy link
Contributor

Good stuff @angadn, thanks for the changes!

@twerkmeister twerkmeister merged commit 6554fec into RasaHQ:master May 12, 2017
vcidst pushed a commit that referenced this pull request Mar 5, 2024
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.

None yet

3 participants