Add explicit GPU models for all makes #111
Conversation
| @@ -121,8 +121,12 @@ class NodePool: | |||
| disk_size: int | None = None | |||
| disk_type: str | None = None | |||
|
|
|||
| gpu: int | None = None | |||
There was a problem hiding this comment.
You cannot just drop these fields, it breaks SDK/CLI.
Some migration procedure is required.
I suggest keeping gpu / gpu_model here for a while (say, until the New Year).
gpu could be a sum of amd, intel and nvidia.
gpu_model could be a first available model or concatination of available modes. We don't parse these values, so nvidia-tesla-k80+amd-rizen-xxx would be fine.
Later, when all clients update CLI/FLOW on their machines, we can drop these fields.
There was a problem hiding this comment.
@asvetlov I've restored both fields for now to not break the compatibility with the SDK.
Just FYI:
gpu was already dropped from the platform config api at April: https://github.com/neuro-inc/platform-config/commit/fc83088224859d5e81b35beb426069888e05c336#diff-61fdfd6f40520f332212d51ba625421a707ce6ff111f58610dd10c84b27b1d9b
If gpu property is sent to the API, it's converted by the API to an nvidia_gpu under the hood - https://github.com/neuro-inc/platform-config/commit/fc83088224859d5e81b35beb426069888e05c336#diff-c2827afbcb2bed90d9593131e0055607f29b65656938e4b93d972c2d40c7e27eR118
There was a problem hiding this comment.
I assume that gpu property is actually blank in a DB since April? @zubenkoivan can you elaborate please?
There was a problem hiding this comment.
In April we renamed gpu to nvidia-gpu and added two other gpu kinds. Because before the April we didn't have non-nvidia systems.
I like your approach for keeping gpu as an alias for nvidia, e.g. nvidia_gpu=payload.get("nvidia_gpu") or payload.get("gpu"),
There was a problem hiding this comment.
Should NodePoolOptions and Resource have triple nvidia/amd/intel values like NodePool?
If not please elaborate why.
There was a problem hiding this comment.
@asvetlov
Resource is used for idle jobs only. I've asked @zubenkoivan about that and he mentioned that it is not required to have a different manufacturers support there.
NodePoolOptions are used only be a cloud providers. As far as I understand - we don't need to add such support for clouds.
@zubenkoivan please correct me if I'm wrong.
There was a problem hiding this comment.
correct, we don't need to support this for NodePoolOptions and Resource classes
parikls
force-pushed
the
explicit-gpu-make-and-model-f606fcc4960f
branch
from
d7cf539
to
7ba9d6d
Compare
for more information, see https://pre-commit.ci
No description provided.