Switch to go-nvml instead of gpu-monitoring-tools #12
Conversation
inspired by elezar 47f8fd5 elezar@47f8fd5 Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Evan Lezar <elezar@nvidia.com>
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Let's name the internal package nvml to reduce the change set as a first step.
| "runtime" | ||
|
|
||
| "github.com/NVIDIA/gpu-monitoring-tools/bindings/go/nvml" | ||
| "github.com/NVIDIA/go-gpuallocator/internal/gpulib" |
There was a problem hiding this comment.
Note: if we use:
| "github.com/NVIDIA/go-gpuallocator/internal/gpulib" | |
| nvml "github.com/NVIDIA/go-gpuallocator/internal/gpulib" |
or even rename the internal package to nvml here it should highlight changes not related to this renaming below.
There was a problem hiding this comment.
I would argue we just add them to go-nvlib's nvml wrapper directly and open a separate PR there
There was a problem hiding this comment.
Sure. That's fine too. My point is that I don't want to have changes due to the rename in the same commit as this makes things difficult to review.
There was a problem hiding this comment.
@klueska Your point is to not commit the internal/gpulib here, but to merge the functionality in there into go-nvlib/pkg/nvml ? and remove from this PR
| "fmt" | ||
|
|
||
| "github.com/NVIDIA/gpu-monitoring-tools/bindings/go/nvml" | ||
| "github.com/NVIDIA/go-gpuallocator/internal/gpulib" |
There was a problem hiding this comment.
Same comment on renaming. That would mean that only the import statement changes in this file.
| runtime.SetFinalizer(allocator, func(allocator *Allocator) { | ||
| // Explicitly ignore any errors from nvml.Shutdown(). | ||
| _ = nvml.Shutdown() | ||
| // Explicitly ignore any errors from gpulib.Shutdown(). |
There was a problem hiding this comment.
Renaming to nvml would mean that we don't have any changes here.
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
ArangoGutierrez
force-pushed
the
go-nmvl
branch
from
0448876 to
1ef5d10
Compare
|
Closing in favour of #13 |
3 participants
inspired by elezar 47f8fd5 elezar@47f8fd5