-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update dockerfile and add docker build action #3283
Conversation
NB: this change basically gets rid of the user and security / "OCI Standards" changes introduced in #2961 The user also needs a more modern version of docker to use multi-stage builds / Not that I'm advocating for restoring the cited changes / user account stuff, but as a nerfstudio user it's really confusing that other PR was accepted in the first place 🤷♂️ |
I actually think changing user is not a good practice. We would require uses to build their own images with their own user id? Seems like this is not ideal for releasing a docker image. Also, I though using sudo is a bad practice for docker nowadays. Also, while #2961 works for podman, it breaks apptainer which I think is more common, and the current solution could also work rootless in podman (just running image, not sure about the compilation). |
FWIW The main case where I've seen a user-ful / sudo docker image helpful at scale was in large teams where a running docker container would need to connect to external developer services and thus a default user account helped provide basic / canonical auth. But this is an open source project w/out public dev services and so I don't recommend it (and TBH I could not grok the arguments given in #2961 ). I would advocate the nerfstudio image be fairly vanilla and then the end user can compose / build atop the image whatever accounts and such they need. E.g. much of the changes in #2961 could have been an add-on / community plugin, with perhaps also a demo of actually using those features. (I've never seen podman / apptainer required at scale, but I guess some contemporary GPU clusters do that because hardware drivers are hard to manage). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(aside from one comment I tried this and it looks good to me!)
f9b426b
to
39b3df8
Compare
Do we want to build with every push into main or with every release? |
Currently we have: What would that look like if we built with every commit? |
We would still have |
In that case doing on every commit makes sense to me! If there are Docker-related problems it'd also help us catch them earlier. (as opposed to only testing the Docker action when we do a release) |
Seems like the gsplat build is running out of CPU/mem...
Does anyone have an idea? Perhaps limit the number of CPU cores for the gsplat build? |
@jkulhanek it seems like setting |
|
I've changed the settings on nerfstudio docker package to inherit the organization permissions and I've locked myself out of it. @brentyi, can you please re-add me as the admin of the package? The build was failing on write permissions which I hope to have fixed before locking myself out. |
Should be done! |
Which version of gsplat do we want to build with? Last main, last released or some fixed version (1.0.0) to match pip install? |
@jkulhanek thanks for your excellent work. I pulled docker://ghcr.io/nerfstudio-project/nerfstudio:pr-3283 with singularity and it works well 👍 Your work towards reducing image size seems to have payed off. The image is just 5GB instead of 8 - 11GB. |
83c9aea
to
95dd640
Compare
If the build passes it should be ready to be merged. I've updated the docs. |
@jkulhanek the image attestation failed. Any idea why? |
Looks good. Merging. |
Thanks for your efforts here @jkulhanek!! |
This PR updates current Dockerfile:
It also adds a github action to build the docker image and push it to associated NerfStudio ghcr packages repository. (We need to enable this first, however).