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

container: --gpus support #1714

Merged
merged 1 commit into from
Mar 21, 2019
Merged

container: --gpus support #1714

merged 1 commit into from
Mar 21, 2019

Conversation

tiborvass
Copy link
Collaborator

@tiborvass tiborvass commented Mar 5, 2019

Signed-off-by: Tibor Vass [email protected]

Closes #1200

Dependent on moby/moby#38828

Most common usecase: docker run --gpus all
Otherwise can be further specified: docker run --gpus 2,driver=nvidia,capabilities=compute

opts/gpus.go Outdated Show resolved Hide resolved
opts/gpus.go Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 12, 2019

Codecov Report

Merging #1714 into master will increase coverage by 0.17%.
The diff coverage is 67.18%.

@@            Coverage Diff             @@
##           master    #1714      +/-   ##
==========================================
+ Coverage   56.21%   56.38%   +0.17%     
==========================================
  Files         307      308       +1     
  Lines       21225    21241      +16     
==========================================
+ Hits        11931    11977      +46     
+ Misses       8427     8390      -37     
- Partials      867      874       +7

@tiborvass tiborvass force-pushed the nvidia-gpu branch 2 times, most recently from 632b386 to 9621d3f Compare March 14, 2019 05:43
@tiborvass tiborvass changed the title [WIP] Add --gpu support [WIP] Add --gpus support Mar 14, 2019
@tiborvass tiborvass changed the title [WIP] Add --gpus support container: --gpus support Mar 19, 2019
@tiborvass tiborvass marked this pull request as ready for review March 19, 2019 19:01
opts/gpus.go Outdated Show resolved Hide resolved
@tiborvass
Copy link
Collaborator Author

tiborvass commented Mar 19, 2019

Ping @thaJeztah @vdemeester @cpuguy83 @silvin-lubecki I revendored and it's ready for review.

@RenaudWasTaken
Copy link
Contributor

Looks good to me!

@thaJeztah
Copy link
Member

This can be rebased now (moby vendor bump was merged)

@tiborvass tiborvass force-pushed the nvidia-gpu branch 4 times, most recently from 308c073 to 3d2b393 Compare March 20, 2019 21:17
@tiborvass
Copy link
Collaborator Author

Ping @thaJeztah @silvin-lubecki @vdemeester updated

opts/gpus.go Outdated Show resolved Hide resolved
opts/gpus.go Outdated Show resolved Hide resolved
opts/gpus.go Outdated Show resolved Hide resolved
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Some nits, notes, and suggestions

Some follow-ups needed to add Bash-completion and reference docs (I know we want to get this in before the release, so I'd be ok with doing that in a follow-up)

opts/gpus.go Outdated Show resolved Hide resolved
opts/gpus.go Outdated Show resolved Hide resolved
opts/gpus.go Outdated Show resolved Hide resolved
opts/gpus.go Outdated Show resolved Hide resolved
opts/gpus.go Outdated

// Type returns the type of this option
func (o *GpuOpts) Type() string {
return "gpuRequest"
Copy link
Member

Choose a reason for hiding this comment

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

perhaps gpu-options this is what's shown in --help output; also looks like we don't use camelBack there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm putting gpu-request

cli/command/container/opts.go Outdated Show resolved Hide resolved
@tiborvass
Copy link
Collaborator Author

@thaJeztah @silvin-lubecki thank you for the review, i updated the PR accordingly.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

two more ideas, but LGTM

cli/command/container/opts.go Outdated Show resolved Hide resolved
opts/gpus.go Outdated Show resolved Hide resolved
@tiborvass
Copy link
Collaborator Author

@thaJeztah updated

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

SGTM, but @tiborvass can I HAZ tests on gpus.go ? 👼

@tiborvass
Copy link
Collaborator Author

@vdemeester added a couple. The most important one is --gpus all

Signed-off-by: Tibor Vass <[email protected]>
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@mviereck
Copy link

mviereck commented Aug 21, 2019

  • It seems that the --gpus option is tied to NVIDIA hardware only and fails with other GPU vendors.
  • It seems that the --gpus option is tied to NVIDIA docker images only and fails with other images.

Am I right? If yes, this new option is a shame.
Please add GPU support for all GPU vendors. Seriously.

EDIT:
Sorry, was intended to be posted in #1200

@RenaudWasTaken
Copy link
Contributor

Hello @mviereck !

The --gpus option is implemented more as a plugin than as a "vendor-lock-in". For exemple it doesn't work until you install the nvidia-container-toolkit (in other words the nvidia plugin).

We have plans to make it more like other plugins (docker plugin install), so that it's easier for everyone to build their own plugins. So far we have lacked time to do so. If you want to help drive this effort we'd be happy to help you (e.g: through review)!
The way the interface for GPUs has been designed should easily map to a REST API.

@mviereck
Copy link

@RenaudWasTaken Thank you for your friendly response!

If you want to help drive this effort we'd be happy to help you (e.g: through review)!

I am not experienced in Go, so I cannot contribute code directly, sorry.
I have opened a new ticket on this topic with some proposals, maybe you want to look at it, too: #2063

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] GPU support in CLI
8 participants