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

Development container image #236

Merged
24 commits merged into from
Dec 8, 2022
Merged

Conversation

ryanolson
Copy link
Contributor

Resolves #203

@ryanolson ryanolson added non-breaking Non-breaking change feature request New feature or request review: ready labels Nov 30, 2022
@ryanolson ryanolson added this to the 23.01 Refactor milestone Nov 30, 2022
@ryanolson ryanolson self-assigned this Nov 30, 2022
@mdemoret-nv mdemoret-nv requested a review from a team as a code owner November 30, 2022 23:59
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

One issue with deleting the old Dockerfile and using this as the new dev container, the instructions for building conda packages are now out of date. The old instructions had the user build the dev container and then run a script. This doesnt work anymore since the Dockerfile has been removed. We can use the new dev container from this PR to build the conda images with a couple of tweaks. This PR should update those instructions so they work again. Instructions are here: https://github.com/nv-morpheus/MRC/blob/branch-23.01/ci/conda/README.md.

I think the easiest solution is to just make a single Dockerfile with a targets for base, driver, and development. Doing it this way has a few advantages:

  1. You wont have to have the yq script look at the yaml file for the base image. It can just build it from earlier stages
  2. External users who dont have NGC setup can still build development containers
  3. We can use the same Dockerfile for building, testing, development, and building conda packages.

scripts/devel.sh Outdated Show resolved Hide resolved
scripts/Dockerfile Outdated Show resolved Hide resolved
@ryanolson
Copy link
Contributor Author

This makes sense. Seems like we should have 4 targets with the additional two being for conda packaging and one for development.

…red common get_image_full_name() function to allow for optional values for the server and tag prefix/suffix; get_image_full_name now requires a positional argument
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
ci/runner/common.sh Outdated Show resolved Hide resolved
@cwharris
Copy link
Contributor

cwharris commented Dec 7, 2022

@mdemoret-nv

We can use the same Dockerfile for building, testing, development, and building conda packages.

What's the advantage to this? It creates one large confusing Dockerfile, leads to oversized images, and doesn't give us the chance to create a fast-building dev image.

@ryanolson
Copy link
Contributor Author

@mdemoret-nv

We can use the same Dockerfile for building, testing, development, and building conda packages.

What's the advantage to this? It creates one large confusing Dockerfile, leads to oversized images, and doesn't give us the chance to create a fast-building dev image.

I'll add the option to skip the conda install if an ENV is set when VS code evaluates the devcontainer or when devel.sh is called.

I'd propose MRC_LOCAL_CONDA_ENV.

This can be a separate PR since I'd like to get the devcontainer kn main so that path is generally persistent across branches. If I'm in a branch without, which is all the other branches right now, then if I have to reload vs code, I have to swap bsck to the dev_image branch to successfully reload.

@cwharris
Copy link
Contributor

cwharris commented Dec 7, 2022

I'll add the option to skip the conda install if an ENV is set when VS code evaluates the devcontainer or when devel.sh is called.

What problem does this solve?

Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

Two small changes

Dockerfile Outdated Show resolved Hide resolved
ci/conda/README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Base: 0.23% // Head: 0.23% // No change to project coverage 👍

Coverage data is based on head (49612c3) compared to base (6e97e7f).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##           branch-23.01    #236   +/-   ##
============================================
  Coverage          0.23%   0.23%           
============================================
  Files               349     349           
  Lines              9898    9898           
  Branches           4807    4807           
============================================
  Hits                 23      23           
  Misses             9875    9875           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ryanolson
Copy link
Contributor Author

@gpucibot merge

@ryanolson ryanolson dismissed cwharris’s stale review December 8, 2022 00:05

all have been addressed

@ryanolson
Copy link
Contributor Author

@gpucibot merge

@ghost ghost merged commit c4f1c0d into nv-morpheus:branch-23.01 Dec 8, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Non-breaking change
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEA] Update Development Image
3 participants