Migrate to go nvml #13
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elezar
force-pushed
the
migrate-to-go-nvml
branch
from
8298aa1 to
bcec1e9
Compare
elezar
commented
Nov 2, 2023
Member
Author
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
This is a WIP. We denfinitely need some more work.
| "github.com/NVIDIA/gpu-monitoring-tools/bindings/go/nvml" | ||
| // TODO: We rename this import to reduce the changes required below. | ||
| // This can be removed once the link-specifics have been migrated into go-nvlib. | ||
| nvml "github.com/NVIDIA/go-gpuallocator/internal/links" |
Member
Author
There was a problem hiding this comment.
This only provides constants as it stands.
gpuallocator/device.go
Outdated
| device.Device | ||
| Index int | ||
| Links map[int][]P2PLink | ||
| // We cache certain values for the device. |
Member
Author
There was a problem hiding this comment.
I would like to avoid embedding all these public symbols, but wanted to limit changes to the other code.
elezar
force-pushed
the
migrate-to-go-nvml
branch
from
bcec1e9 to
35ca8b6
Compare
elezar
force-pushed
the
migrate-to-go-nvml
branch
from
35ca8b6 to
48f9f9f
Compare
elezar
force-pushed
the
migrate-to-go-nvml
branch
from
48f9f9f to
9c4b7d5
Compare
elezar
requested review from
ArangoGutierrez,
cdesiniotis,
klueska and
tariq1890
elezar
commented
Nov 14, 2023
elezar
force-pushed
the
migrate-to-go-nvml
branch
2 times, most recently
from
6d06cbe to
55c2315
Compare
tariq1890
reviewed
Nov 15, 2023
elezar
force-pushed
the
migrate-to-go-nvml
branch
from
55c2315 to
c950cb4
Compare
elezar
commented
Nov 16, 2023
elezar
commented
Nov 16, 2023
elezar
commented
Nov 16, 2023
elezar
commented
Nov 16, 2023
elezar
commented
Nov 16, 2023
elezar
commented
Nov 16, 2023
elezar
commented
Nov 16, 2023
elezar
force-pushed
the
migrate-to-go-nvml
branch
4 times, most recently
from
7e3db16 to
6450bbf
Compare
cdesiniotis
reviewed
Nov 16, 2023
elezar
force-pushed
the
migrate-to-go-nvml
branch
from
6450bbf to
e77fc49
Compare
klueska
reviewed
Nov 17, 2023
Comment on lines
17
to
33
| type Device struct { | ||
| *nvml.Device | ||
| device.Device | ||
| Index int | ||
| Links map[int][]P2PLink | ||
| // The previous binding implementation used to cache specific device properties. | ||
| // These should be considered deprecated and the functions associated with device.Device | ||
| // should be used instead. | ||
| cachedDeviceInfo | ||
| } | ||
|
|
||
| // The previous binding implementation used to cache specific device properties. | ||
| // We collect these into a separate type to allow for embedding and simpler migration from them. | ||
| type cachedDeviceInfo struct { | ||
| UUID string | ||
| PCI struct { | ||
| BusID string | ||
| } | ||
| CPUAffinity *uint | ||
| } |
Contributor
There was a problem hiding this comment.
I would have expected it to be like:
type Device struct {
nvlibDevice
Index int
Links map[int][]P2PLink
}
type nvlibDevice struct {
device.Device
// The previous binding implementation used to cache specific device properties.
// These should be considered deprecated and the functions associated with device.Device
// should be used instead.
UUID string
PCI struct {
BusID string
}
CPUAffinity *uint
}
Member
Author
There was a problem hiding this comment.
OK. Will update. One thing I was just wondering was whether Index should also be in the nvlibDevice struct, but I'm going to leave it where it was previously for now.
gpuallocator/device.go
Outdated
| return nil, fmt.Errorf("failed to get device pci info: %v", ret) | ||
| } | ||
|
|
||
| linkedDevice := Device{ |
gpuallocator/device.go
Outdated
| device, err := nvml.NewDevice(uint(i)) | ||
| var devices DeviceList | ||
| err := o.devicelib.VisitDevices(func(i int, d device.Device) error { | ||
| linkedDevice, err := newDevice(i, d) |
This change migrates from the gpu-monitoring-tools bindings for NVML to those abstracted by go-nvlib. The functions such as peer-to-peer interconnectivity not provided by go-nvlib are implemented in a new interal links package. The functions and types here can be considered for future migration to go-nvlib. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
force-pushed
the
migrate-to-go-nvml
branch
from
e77fc49 to
a73b9c4
Compare
klueska
approved these changes
Nov 17, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have created NVIDIA/go-nvlib#7 to include the constants and NVML functions that we require here.
I have also created https://gitlab.com/nvidia/kubernetes/device-plugin/-/merge_requests/328 to update the device plugin to use the new API for managing Linked devices.