-
Notifications
You must be signed in to change notification settings - Fork 39
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
remove development target, install all packages in postcreatecommand #679
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #679 +/- ##
=======================================
Coverage ? 86.25%
=======================================
Files ? 119
Lines ? 7208
Branches ? 0
=======================================
Hits ? 6217
Misses ? 991
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
Signed-off-by: Peter St. John <pstjohn@nvidia.com>
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.
This deletes interna/scripts. If this is intentional, could you please:
- update the README with alternative instructions for building images and developer environment.
- update the PR description to describe the changes to developer workflow (e.g. instead of using "build_dev_image.sh, we prefer... blah"
- describe the major changes to Dockerfile in a bullet list? I think it would be easier to look at the change and see something like, 1) removed development target, 2) release is built from dev 3)
Or something else if it makes more sense. Ideally both the PR description and the README would make clear both the changes from this script and how we expect workflows to change.
Discussed with @pstjohn, ideally we can preserve our Implementation is super flexible (e.g. a third build path in the Dockerfile called
Part of my (maybe illogical) fear is that Originally, this PR was meant to fix an issue where
No rush! We have WAR for bionemo-noodles and a working setup, let's get it right and hopefully not worry about this for a while! |
A fairly significant refactor of our release and dev targets in the Dockerfile, and removes the "development" target. (Along with some scripts that reference it, and the old
uv.lock
file we don't use).Addresses some of the concerns posted in https://nvidia.slack.com/archives/C074Z808N05/p1738787505094379, specifically the error
That arose from deleting bionemo modules in creating the dev container, but not installing the editable packages with their dependencies.
This changes the "release" target to run as the ubuntu user by default, so that files created by this image don't end up owned as
root
. It also changes the devcontainer to install + remove our packages by default, but rather just install everything we need inpostCreateCommand
, which should mimic the release container environment more closely.