Update to incorporate go-nvml updates to expose interface types #55
Conversation
klueska
force-pushed
the
update-go-nvml
branch
6 times, most recently
from
aa5891f
to
bea8b3e
Compare
klueska
changed the title
There was a problem hiding this comment.
I think the initial integration looks good. There are some cases where it may make sense to use go-nvlib functionality, but I think this can be done as a follow-up.
The tests also call out some places where specific mock device implementations that could be consumed by other tests may be useful.
| @@ -47,21 +47,21 @@ func NewMockNvmlMigModeManager(nvml nvml.Interface) Manager { | |||
|
|
|||
| func (m *nvmlMigModeManager) IsMigCapable(gpu int) (bool, error) { | |||
There was a problem hiding this comment.
Will there be a follow up to use the equivalent functionality from go-nvlib?
There was a problem hiding this comment.
It's probably a good idea to audit this code to see what can be used from go-nvlib (or added to go-nvlib as necessary). This code predates go-nvlib so I'm sure there's lots of things that could be simpified now.
| @@ -52,38 +52,38 @@ func NewMockNvmlLunaServer() *nvmlMigModeManager { | |||
|
|
|||
| func (d *mockNvmlA100Device) SetMigMode(mode int) (Return, Return) { | |||
There was a problem hiding this comment.
At which point do we replace the usages of mockNvmlA100Device with those from the mock package? (or implement them there)?
There was a problem hiding this comment.
What do you mean by "at which point"? At which point in the current code? They are not replacements for the ones in the mock package, but rather extend from them to provide logic necessary for testing the abstractions in mig-parted specifically.
There was a problem hiding this comment.
This was unclear from my side. The question should have been whether it makes sense to move these abstractions to a common location. And if it makes sense, does it have to be done as part of this PR. I think your answer makes it clear that these are not as common as I thought on the first read-through.
|
Do we want to include this in the upcoming 0.7.0 release? |
I'll leave this to @klueska to decide. I think we said that it was not a hard requirement for the 0.7.0 release. |
|
I think we should explicitly leave it out of the imminent release (in case there are any unexpected issues it introduces). |
klueska
force-pushed
the
update-go-nvml
branch
3 times, most recently
from
46c1ca1
to
d42258c
Compare
Signed-off-by: Kevin Klues <[email protected]>
|
This is now ready from my perspective. Please review again. |
Closes #57