-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[RFC] GPU support in CLI #1200
Comments
First approach SGTM |
|
Really, if I just had a standard set of things I'd want to include, I'd put it into an variable or a shell function that I can use to inject it into my
The docker client doesn't need to care about this. If the system that the engine is running on doesn't support it (e.g., likely that docker4mac doesn't unless they end up injecting a GPU passthrough or something?) then it would error out from the daemon side, likely at runtime. |
I thought about responding with additional feedback but not even worth it at this point. If your response to someone actually commenting on an RFC (Request For Comments) is some kind of low level trolling plus affirming that's okay via a laughing face from the original RFC author why even bother? |
I made a lighthearted joke at no ones expense and gave some real feedback. This isn't my proposal. |
@flx42 Ya, I like the suggested approach and namespaced keys for the opts. I'm guessing it would all people to get a quick default, |
Maybe a compose section looks something like: - gpus:
- type: "nvidia"
- devices:
- 0
- "guid"
- opts:
- "..." |
Yeah, but not sure how we are reconciling this with what's supported on Swarm. |
The alternative would be to shove this into the "mounts" API, though we can still keep the somewhat specialized CLI for ease of use. |
I laughed because it's true, that's something we might need to do, and we will call that backwards-compatibility :). I apologize if it appeared dismissive of your reply. I think, as mentioned above, we need a sensible default, therefore we need to make sure |
@flx42 Would it make sense to take that a step further and support something like |
@cpuguy83 I was also wondering if we should include a special value to select all the GPUs: --gpus nvidia # your proposal
--gpus nvidia=* # might be problematic with shell globbing, but --device-cgroup-rule is similar
--gpus nvidia=all # ambiguous with GPU ID I'm fine with your option. |
I think @cwgem does this work in terms of simplifying the experience? Separate topic: Where should this fit into the API? |
@cpuguy83 started the API discussion here: moby/moby#37434 |
Do you think we should make the CLI more abstract where you don't specify GPU device id but number of gpus added? Or is this not a good abstraction for this layer of the stack? |
I think this would cause some issues, this doesn't feel to be the right place in the stack for resource management/allocation.
|
Happy to see this moving, and to see that the (technical) design is starting to take form! Just a quick first look (I'll have to give this a bit more thinking);
Given that a CLI can be used against different versions of the daemon, we try to avoid validation as much as possible in the CLI, and delegate this to the daemon (i.e.; the API can return an error if a vendor and/or option is not supported); this way any client using the API would also be covered. This is similar to (e.g.) a specific logging-driver not being available on a daemon question: is
Same as above; validation of what's valid should be handle by the daemon/runtime. Possibly (and, if needed), a minimal validation (e.g., length, accepted characters - if we can define) could be possible to fail early
Some questions/remarks here;
I agree the CSV format can become ugly/verbose. It's also the most non-ambiguous, extensible format currently (and would address my questions above for the "suggested approach")
If we support passing CSV arguments multiple times, expansion would not be needed;
I expect users that require many options to not be typing these by hand, but using (e.g.) a The format in a compose file could be something like; gpus:
- identifier: GPU-abcd
type: nvidia
options:
- capabilities=compute
- kmods=true
- identifier: 0
type: nvidia
options: ["capabilities=compute", "kmods=true"\
- identifier: 1
type: intel
options:
- "..."
We should definitely avoid this format; colon-delimited, positional options have proven to be very problematic, and can easily become ambiguous
This would not be extensible, and would require the CLI to be tied to the daemon version (and supported options) |
That makes sense, OK.
Today the implementation for NVIDIA relies on the API provided by containerd. Other vendors could integrate either at the
Even if we wanted to, it would not be possible for GPU identifiers since you might be on a MacOS machine connected to a remote docker daemon.
Sure:
That makes sense, yes, look at my example above for limiting GPU memory:
But I've excluded this feature from the initial API, I don't think we need it right away.
The prefix would solve the problem indeed:
The CLI will see the following (because
Which is not what we want, now it's a quoting nightmare. cc @shin- since compose was mentioned multiple times already :) |
The problem I see with the numeric indexes is that (translating that to the compose file), options also have to be provided in the same order/position; For example;
Would (if I see it correctly) become something like gpus:
types:
- nvidia
- nvidia
- nvidia
identifiers:
- 0
- 3
- 4
opts:
-
-
- ["limit_mem=2GB", "other_opt=foo"] |
@thaJeztah I'm not sure I understand, why is the CLI format impacting the docker-compose format? Does it matter as long as we can have the same semantic with the API behind? I think the best compose format would be what you described above: gpus:
- identifier: GPU-abcd
type: nvidia
options:
- capabilities=compute
- kmods=true
- identifier: 0
type: nvidia
options: ["capabilities=compute", "kmods=true"\
- identifier: 1
type: intel
options:
- "..." |
Perhaps it's not a strict rule, but so far, there's been a mapping between long-form options on the CLI and how they are used in the Docker Compose file e.g. volumes:
- type: foo
source: bar
target: baz |
So to recap a bit the conversation if we want the compose format to look like this gpus:
- identifier: GPU-abcd
type: nvidia
options:
- capabilities=compute
- kmods=true
- identifier: 0
type: nvidia
options: ["capabilities=compute", "kmods=true"]
- identifier: 1
type: intel
options:
- "..." The CLI would look like this: Declining it into multiple:
Compared to today:
This looks fine for the compose file, but I'm a bit uncomfortable with having to write everything on the CLI. |
@RenaudWasTaken Why wouldn't that be multiple Also, can we |
@cpuguy83 my thinking was that you would use multiple Translating to the following compose: gpus:
- identifier: GPU-abcd
type: nvidia
options:
- capabilities=compute
- kmods=true
- identifier: 0,1,2
type: intel
options:
- capabilities=compute
- kmods=true But if you just want to expose 3 nvidia-gpus it would look like this: Translating to the following compose: gpus:
- identifier: 1,2,3
type: nvidia
options:
- capabilities=compute
- kmods=true Also I'm wondering if Side note I don't think that value can be either a string or an array? What do you think? |
This is my proposal based on conversations I had with @RenaudWasTaken Design taken:
Proposed syntax:
In the first iteration, let's assume there be only one GPU provider per engine ("nvidia"). Most common usecases:
In most of the cases the above should be sufficient. It feels simple, as portable as possible (id is not portable) and allows for more complex queries. Less common usecases:
Future (just to show that the following is possible)
Things I need to think more about:
|
Very quick glance at your suggestion, but one thing that's a bit unclear how that'd work;
If I think for "per brand" options to be an option, we need a defined option that specified; only take this into account if the branch matches, and if so, |
This one may be tricky as well (as If we separate them, (possibly) we could define constraints that are driver/vendor-agnostic |
@tiborvass thanks for your detailed API post, I think it would work pretty well for GPUs and it is pretty straightforward to use in most cases! @thaJeztah can you give an exemple of the brand problem you are surfacing? I'm not completely grasping the issue. |
Oh! Sorry, I forgot to update; had a quick chat on Slack with @tiborvass, and I was confusing
Because of that confusion I thought that This does bring up one thing (briefly touched on in #1200 (comment) as "I know it can fail if provider decides constraints are not met"); what is the desired outcome of (In a situation where a node can have a mixture of
If
Downside of the "namespace" approach, is that, while it makes sense if you want to mix/mash GPUs (didn't set If we really don't want to namespace/prefix options, we could of course leave the meaning of
I'm a bit unclear on;
What is the expected result if multiple
(Perhaps this can be addressed by setting |
Am I right? If yes, this new option is a vendor-lock-in and a shame. |
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)! |
@RenaudWasTaken Thank you for your friendly response!
I am not experienced in Go, so I cannot contribute code directly, sorry. |
The goal of this RFC is to discuss how we can expose the GPU support that was recently added to containerd by @crosbymichael in this PR: containerd/containerd#2330
Since there are multiple ways of implementing this, I think it makes sense to start with the user experience, i.e. the CLI. If we can agree on the format, the next step will be that I write a PR for API changes in the
github.com/docker/docker/api/types
package. My rationale being that if we decide to not allow some configuration options in the CLI, it would be useless to haveapi/types
expose it. As a concrete example, if we don't allow passing options on a per-GPU basis, it shouldn't be part of the API.After discussing with @crosbymichael, he advised to limit the support to GPUs. We are excluding other types of specialized hardware such as FPGAs, InfiniBand, ASICs... from this RFC.
This RFC introduces some vocabulary, then presents the approach I find is the most compelling.
GPU Vendors
e.g.
nvidia
,intel
oramd
.com.nvidia
) is too verbose and should probably be avoided for a CLI.GPU identifiers
--with-gpus
, users should be able to select a subset of all GPUs, similarly to--cpuset-cpus
:nvidia=0,1,2,3
--cpuset-cpus
), GPUs can have a string representation, for instance a UUID:nvidia=GPU-fef8089b-4820-abfc-e83e-94318197576e
GPU options
nvidia.capabilities = compute
.limit_mem[0] = 2GB limit_mem[1] = 6GB
.Suggested approach
Add two CLI options:
--gpus
for specifying a list of GPUs (like--cpuset-cpus
)--gpus-opt
for specifying a list of GPU options (like--storage-opt
)The important element being the vendor name, which serves as a key for specifying options and identifiers.
I think this is easy to read since you don't have to pack everything in a single argument.
If we want per-GPU options, we can introduce some sort of indexing in
--gpus-opt
:Discarded approaches
I think the approach above is the best, but here are some thoughts on other possibilities.
Single CSV argument
Similar to
--mount
, add a new argument--gpus
which expects a CSV value.This is explicit and everything is contained in one option, but it's also bizarre since we have nested equal signs, and we have to expand the list of GPU identifiers as multiple arguments, to avoid needing complicated quoting from the shell.
A follow-up question would then be: can you specify multiple times the same vendor? If yes, what if the listed options don't match?
Similar to
--device
Add a new argument
--gpus
which expects 2 or 3 strings separated by:
Less problematic than approach above, but hard to read.
NVIDIA specific
Same than the suggested approach, but without being generic:
It removes the need to use the vendor name as the key, and is fine for now, but is likely to be removed in the future if we add more vendors. Also, this doesn't match the approach used by the containerd PR.
The text was updated successfully, but these errors were encountered: