Docker for build dependencies only - WIP #718
Conversation
Extract docker specific portion of README.md into ci/README.md. More changes to come, but this is setting the stage.
Haven't yet removed the temporary copy of POCS.
This is the standard convention.
Codecov Report
@@ Coverage Diff @@
## develop #718 +/- ##
===========================================
- Coverage 80.2% 80.18% -0.02%
===========================================
Files 61 61
Lines 5096 5096
Branches 697 697
===========================================
- Hits 4087 4086 -1
Misses 816 816
- Partials 193 194 +1
Continue to review full report at Codecov.
|
|
Please take a look and let me know what you think... or try it out yourself. |
There was a problem hiding this comment.
Is this to be used only for travis? I had some comments about having multi-stage builds and the use of RUN. If this is to be only used for travis those might not as apply as much.
This kind of seems like a WIP? I can't quite tell what the scope of this PR should be or how it relates to #712 and #713.
- README cleanups, including the removal of duplicates
| # | ||
| # Setup directories needed by PANOPTES. Other scripts, such as | ||
| # install-dependencies.sh, expect to find these directories. | ||
| # We do NOT parameterize these paths (i.e. don't use environment |
There was a problem hiding this comment.
Why not just allow this from $PANDIR?
There was a problem hiding this comment.
I don't understand the question. Could you clarify?
There was a problem hiding this comment.
It just seems to hardcode the directory so I was wondering the reason.
ci/full-dependencies/Dockerfile
Outdated
| # these, the cached image layer is used). | ||
|
|
||
| # Update the information we know about package versions. | ||
| RUN apt-get update --fix-missing |
There was a problem hiding this comment.
My understanding is that in general you want to minimize the number of RUN, COPY, and ADD steps as they each create new layers and thus inflate the size. I realize we might be caching them but it still causes the overall container size to be larger.
Unless this is being used as part of a multi-stage build? But if that is the case it seems like we should stage it up even more.
There was a problem hiding this comment.
I'd rather not worry about that until we have a generally working process. You're right that it does increase the number of cached intermediates, but it turns out that is really valuable when working on changing the later steps in the Dockerfile.
ci/full-dependencies/Dockerfile
Outdated
|
|
||
| # First we copy the files and directories least likely to change while | ||
| # iterating on the building of the Docker image. | ||
| COPY .codecov.yml .coveragerc .gitignore .gitmodules .pycodestyle.cfg \ |
There was a problem hiding this comment.
I don't understand why the selective copying? Is this also for caching purposes?
There was a problem hiding this comment.
It avoids copying .git, build, htmlcov, etc. I put in the full list of files and directories in the repo simply to avoid getting ridiculous about selectivity... and so that it can be automated. I want to be able to run scripts/download_support_files.sh, which means I'll probably need to run setup.py, but I probably don't really want the full impact of that cached (i.e. a modified miniconda tree), so some thought will be needed.
| (set -x ; ${CHOWN} -R "${PANUSER}:${PANUSER}" /var/panoptes) | ||
| (set -x ; ${CHMOD} -R 755 /var/panoptes) | ||
|
|
||
| for SUBDIR in POCS PAWS logs astrometry/data |
There was a problem hiding this comment.
These directories all seem a) hard-coded b) buried in the script.
There was a problem hiding this comment.
These are the directories that currently checked by some of our install or download scripts. I'd appreciate suggestions regarding improving this, as I don't have one at the moment.
| @@ -0,0 +1,47 @@ | |||
| #!/bin/bash -ex | |||
There was a problem hiding this comment.
A copy from one of those, esp. since I think this may be a more fruitful direction. I like that I can iterate locally on Dockerfile, unlike working with the travis file.
| return | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Do you still need everything below if the script is already installing?
There was a problem hiding this comment.
Sorry, rushed to get the dockerfile to you, but this part is clearly a WIP. In particular, IFF we are going to move in the direction of docker, then we don't need to have a single file that one can download in order to do an install, so we could break this huge file up into various scripts with a single coordinator. Thoughts?
There was a problem hiding this comment.
IFF we are going to move in the direction of docker
I still don't understand if this question regarding docker is for testing (on travis) or if it applies more generally to an install method. Or even more sorta-generally as an image that could be used, e.g. for compute instance images, travis, etc.
Breaking it up would favor either approach as you could have different files (or I think there are build "flavors" or something) for different targets but build them from the same intermediary stages.
Co-Authored-By: jamessynge <[email protected]>
Split apt-packages-list.txt into two files, one for use in ci, the other with the remaining packages. This is still a WIP.
jamessynge
changed the title
|
I'm going to close this, and will send other smaller PRs when ready. |
Incomplete, but almost there.