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

[Into Pixi PR #4] Fix Dockerfiles #5

Merged
merged 11 commits into from
Aug 9, 2024
Merged

[Into Pixi PR #4] Fix Dockerfiles #5

merged 11 commits into from
Aug 9, 2024

Conversation

jayqi
Copy link
Member

@jayqi jayqi commented Aug 9, 2024

This is a PR into the branch for #4 to fix some issues with the Dockerfiles.

See comments for explanations.

@jayqi jayqi reopened this Aug 9, 2024
@jayqi jayqi requested a review from klwetstone August 9, 2024 02:21
COPY apt.txt apt.txt
RUN apt-get update --fix-missing \
&& apt-get install -y apt-utils 2> /dev/null \
&& xargs -a apt.txt apt-get install -y \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/* /apt.txt

COPY --chown=$MAMBA_USER:$MAMBA_USER pixi.lock /tmp/pixi.lock
COPY --chown=$MAMBA_USER:$MAMBA_USER pixi.toml /tmp/pixi.toml
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you tried running your original, but pretty sure you'd get an error because the MAMBA_USER environment variable does not exist.

The mambaorg/micromamba base image is the thing that sets these environment variables and creates this user.

https://github.com/mamba-org/micromamba-docker/blob/8a73165bd53a15445afd8e9c51a2137d1902b0ce/Dockerfile#L42-L49

Since we're not using it anymore, then we don't have a non-root runtime user. We need to create it ourselves. The lines further up in the Dockerfile create the user.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh I ran it without issue -- I wonder if it was somehow cached from when I built the image previously using the micromamba base. Either way, makes sense!

COPY pixi.toml ./pixi.toml

RUN pixi install -e ${CPU_OR_GPU} --frozen \
&& rm -rf /home/${RUNTIME_USER}/.cache/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pixi clean actually deletes the environment you install. This is the way to manually delete the cache directory.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah got it I didn't realize "cleaning the environment" in the pixi docs meant deleting it.

It looks like we could also use the clean cache command, eg pixi clean cache --yes. Would that do the same thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, I think you're right. I didn't find that in --help yesterday.

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
docker run \
--mount type=bind,source="$(shell pwd)"/runtime,target=/tmp \
--rm \
pixi-lock:local
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this line copy pixi.lock over from the docker container back to the host?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--mount type=bind,source="$(shell pwd)"/runtime,target=/tmp \ mounts the local runtime/ directory into the container, so anything the container does happens directly to the host filesystem's runtime/.

@klwetstone
Copy link
Collaborator

@jayqi thank you this looks great and all runs without issue! A couple small question in the comments -- mostly for my understanding, no code changes needed

@jayqi jayqi merged commit b5dbb58 into jyq-pixi Aug 9, 2024
1 of 2 checks passed
klwetstone added a commit that referenced this pull request Aug 16, 2024
* Use pixi for dependency management

* Fix tensorflow

* Move shared dependencies into a base feature

* move pixi files

* remove conda-lock files

* dockerfile scrap work

* update dockerfile

* use pixi docker base image

* work updating test_lockfile

* update make update-lockfiles for pixi

* separate dockerfile to update pixi.lock

* update lockfile

* update makefile

* remove test_lockfile.py adapted for pixi

* makefile logging

* [Into Pixi PR #4] Fix Dockerfiles (#5)

* Fix Dockerfiles

* Fix permissions and directories and stuff

* Update test command

* Fix command ordering

* Use clean cache command

* Add maximize build space action

* Add more root reserve space

* Remove unwanted software directly

* Add some diagnostics

* Print out pixi info

* Fix typo

---------

Co-authored-by: Jay Qi <[email protected]>

* Use a cuda base image

* Hardcode python executable for test

* Remove extra Python

* pass CPU_OR_GPU to entrypoint.sh

* log cpu or gpu in entrypoint

* use jammy for locking

* test pixi.lock update

* hacky pixi dependency fix

* revert pixi.lock

* only log in smoke tests

* fix mounting for lockfile generation

* revert makefile changes for testing

* fix comment

* omit trailing slash

---------

Co-authored-by: Jay Qi <[email protected]>
Co-authored-by: Katie Wetstone <[email protected]>
@klwetstone klwetstone deleted the jyq-fix-dockerfiles branch September 10, 2024 20:21
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.

2 participants