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.

Sign up for GitHub

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

Jump to bottom

GPU requirement and auto-detect NVIDIA extensions #173

Merged
merged 9 commits into from
Nov 11, 2022
Merged

GPU requirement and auto-detect NVIDIA extensions #173

merged 9 commits into from
Nov 11, 2022

Conversation

chris-major-improbable
Copy link
Contributor

@chris-major-improbable chris-major-improbable commented Sep 13, 2022 •
edited
Loading

This PR builds upon the proposal in devcontainers/spec#82
When launching a devcontainer requesting GPUs it also detects the presence of NVIDIA docker extensions, and adds the --gpus all flag.

This means that the same devcontainer can be used by developments that don't all have GPU's.

It also does the equivalent for docker-compose scripts, adding the GPU access to the service specified in the devcontainer.json file

@chris-major-improbable chris-major-improbable requested a review from a team as a code owner September 13, 2022 11:23
chrmarti
chrmarti requested changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have left a comment.

src/spec-node/singleContainer.ts Outdated Show resolved Hide resolved
@chris-major-improbable chris-major-improbable force-pushed the chrism/gpu-requriements branch 2 times, most recently from 5bf01c3 to 5af8294 Compare September 27, 2022 12:00
chrmarti
chrmarti reviewed Oct 3, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Thanks for the update! I have added a few more comments.

src/spec-node/dockerCompose.ts Outdated Show resolved Hide resolved
src/spec-configuration/configuration.ts Outdated Show resolved Hide resolved
src/spec-configuration/configuration.ts Show resolved Hide resolved
chrmarti
chrmarti requested changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Looks good, left a few more comments. Thanks!

src/spec-node/singleContainer.ts Outdated Show resolved Hide resolved
src/spec-node/imageMetadata.ts Show resolved Hide resolved
src/spec-node/dockerCompose.ts Outdated Show resolved Hide resolved
src/spec-configuration/configuration.ts Outdated Show resolved Hide resolved
src/spec-node/dockerCompose.ts Outdated Show resolved Hide resolved
@chris-major-improbable chris-major-improbable force-pushed the chrism/gpu-requriements branch 2 times, most recently from 6dca834 to dfc50d4 Compare October 10, 2022 16:49
chrmarti
chrmarti requested changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

2 more comment. :)

src/spec-node/dockerCompose.ts Outdated Show resolved Hide resolved
src/spec-node/dockerCompose.ts Outdated Show resolved Hide resolved
chrmarti
chrmarti previously approved these changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

LGTM! (Looping in our partner team for cross-review. Will merge after that. Thanks!)

@chris-major-improbable
Copy link
Contributor Author

Hey @chrmarti just seen a lot of test failures on the CI - and slightly at a loss to understand what they mean and why they are failing. I'm happy to take a look, but I'm not sure what the difference between src/test/cli.build.test.ts and src/test/cli.exec.buildKit.1.test.ts is (and how to replicated it locally?)

@chris-major-improbable
Copy link
Contributor Author

I've taken a bit of a closer look, and rebased onto the latest main - and I see the same failures running the tests locally, so I don't think this is a regression from this PR.

@kieferrm kieferrm mentioned this pull request Nov 6, 2022
Closed
16 tasks
@chrmarti
Copy link
Contributor

chrmarti commented Nov 7, 2022

@chris-major-improbable I have merged in the main branch and fixed an issue with the PR I found when looking into the test failures. Please check the above comment from the CLA bot, we must have added that after you submitted the PR. Thanks!

@chris-major-improbable
Copy link
Contributor Author

@microsoft-github-policy-service agree [company="{Improbable Worlds Ltd}"]

@chris-major-improbable
Copy link
Contributor Author

@microsoft-github-policy-service agree company="{Improbable Worlds Ltd}"

@chris-major-improbable
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Improbable Worlds Ltd"

@chris-major-improbable
Copy link
Contributor Author

Thanks for that - looks good to me.
I've merged in the latest and fixed one conflict, and should be good now.

@tbenthompson tbenthompson mentioned this pull request Nov 9, 2022
Closed
chrmarti
chrmarti approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

LGTM! (Will get a second review now.) Thanks!

joshspicer
joshspicer approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@joshspicer joshspicer left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution.

samruddhikhandale
samruddhikhandale approved these changes Nov 10, 2022
View reviewed changes
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the contribution! ✨

@chrmarti chrmarti merged commit f533ca1 into devcontainers:main Nov 11, 2022
@jonnyyu jonnyyu mentioned this pull request Dec 8, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samruddhikhandale samruddhikhandale samruddhikhandale approved these changes

@joshspicer joshspicer joshspicer approved these changes

@chrmarti chrmarti chrmarti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chris-major-improbable @chrmarti @joshspicer @samruddhikhandale