Bake PHOLD into the Docker image - #149
Conversation
waxlamp
left a comment
waxlamp
left a comment
There was a problem hiding this comment.
I was able to build this branch (with the help of the changes in the container-rev-matching branch) and the changes make sense to me.
|
|
||
| # Directory for PHOLD output files | ||
| # DJANGO_PHOLD_OUTPUT_DIR=/tmp/phold_output | ||
| DJANGO_PHOLD_OUTPUT_DIR=/tmp/phold_output |
There was a problem hiding this comment.
Is /tmp the right place to store output?
There was a problem hiding this comment.
I'm not sure if it's the right place, but I can't see a reason why it's the wrong choice either. It's purely a temporary staging location before the parsed data lands in object storage. This does draw my attention to the fact that we do not have a cleanup step though, so in theory /tmp/phold_output could grow unbounded... I think it would make sense to handle that after _ingest_output_file succeeds so we don't keep these files around after we've ingested the results.
|
|
||
| # Number of MPI processes for PHOLD simulations (set to 1 for no MPI) | ||
| # DJANGO_PHOLD_MPI_PROCESSES=4 | ||
| DJANGO_PHOLD_MPI_PROCESSES=4 |
There was a problem hiding this comment.
Why set this at the Django settings level (rather than let it be part of the run configuration)?
There was a problem hiding this comment.
Great question. Do we want to expose this to users? Initially this was the easiest way to get things up and running, and I didn't really consider exposing it because I wasn't sure if we wanted users to have to think about this. Maybe if we don't expose it we do something a little smarter to detect available resources and set a reasonable value, or we just skip that and expose it to users as a part of the run configuration. What do you think?
| # Pin to a specific ROSS commit for reproducible builds. | ||
| ARG ROSS_GIT_REF=dc3a6a056cfc7a5e68f7141f88d8833407599ef8 | ||
|
|
||
| FROM ubuntu:24.04 AS ross-builder | ||
| ARG ROSS_GIT_REF | ||
|
|
||
| RUN apt-get update && apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| git \ | ||
| build-essential \ | ||
| cmake \ | ||
| openmpi-bin \ | ||
| libmpich-dev \ | ||
| libopenmpi-dev \ | ||
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| RUN git clone https://github.com/ross-org/ross.git /ross \ | ||
| && cd /ross \ | ||
| && git checkout "${ROSS_GIT_REF}" \ | ||
| && git submodule update --init --recursive | ||
|
|
||
| RUN cmake -S /ross -B /ross/build \ | ||
| -DCMAKE_BUILD_TYPE=Release \ | ||
| -DROSS_BUILD_MODELS=ON \ | ||
| -DROSS_BUILD_TESTING=OFF \ | ||
| && cmake --build /ross/build --parallel | ||
|
|
||
|
|
There was a problem hiding this comment.
Why build ROSS in a separate image and copy the binary over to the "main" image? Is it to keep future such builds independent of the main image, so that the order of building them doesn't create artificial dependencies for the main image build?
There was a problem hiding this comment.
Yes, essentially. Without the split the containers would have the full build toolchain and the ROSS code, none of which is needed at runtime. Of course this also has the advantage of preventing cache invalidation and rebuilding when an unrelated change is made to one stage or the other.
| mark_run_failed( | ||
| None, RuntimeError("No PHOLD output files were produced"), None, run_id=run.id | ||
| ) |
There was a problem hiding this comment.
My own curiosity: why is None being passed for the request and traceback arguments here? That might indicate that this function is being asked to do different jobs.
There was a problem hiding this comment.
You're correct - this is a case where we should probably extract the actual logic for status updating into a helper so that it is called both here and by Celery (which is what this signature was designed around and is why we're passing None). I'll make that update!
|
|
||
| ## Setup | ||
| 1. **Configure simulation paths**: Set `ROSS_PATH` and PHOLD settings in `dev/.env.docker-compose` | ||
| 1. **Configure simulation settings** (optional): PHOLD settings in `dev/.env.docker-compose`. |
There was a problem hiding this comment.
| 1. **Configure simulation settings** (optional): PHOLD settings in `dev/.env.docker-compose`. | |
| 1. **Configure simulation settings** (optional): Review and change PHOLD settings in `dev/.env.docker-compose`. |
2 participants
_ingest_output_filesnow marks the runFAILEDwhen zero expected output files are found, instead of silently completing with no data.PHOLDagainstOpenMPI; the final image copies just the compiled binary to/opt/ross/phold.${ROSS_PATH:-../ross}bind mountsROSS_PATH