Restored default nvml api

[dschervov]

Restored default nvml api
The original (C) nvml let user specify what
type of temperature sensor he want to query.

This is why we change GO implementation, and
user can call the functions with familiar
manner.

Signed-off-by: Dmitrii Chervov dschervov@yandex-team.ru

[klueska]

The handler type is not meant for an end user.

Users are meant to invoke this as You use it as:

device.GetTemperatureV(sensorType).V1()

And in the future (if there is ever a v2 struct) as:

device.GetTemperatureV(sensorType).V2()

[dschervov]

The handler type is not meant for an end user.

Users are meant to invoke this as You use it as:

device.GetTemperatureV(sensorType).V1()

And in the future (if there is ever a v2 struct) as:

device.GetTemperatureV(sensorType).V2()

I am grateful for the answer! My bad for disturbing you, just wanted to help

[klueska]

No worries. I believe you did in fact uncover a bug, because the input parameters of the struct aren't being passed in like I suggest in my response. So your fix was incorrect, but the bug is real.

[dschervov]

No worries. I believe you did in fact uncover a bug, because the input parameters of the strict aren't being passed in like I suggest in my response. So your fix was incorrect, but the bug is real.

Yeah, i re-open my PR to say about it : - D

I want to fix this bug, can you please help me with youre review? I do my implementation, you say if maybe i need to change something, and we will get great working code (already made the changes ⚡ )

[dschervov]

@klueska , @cdesiniotis jentle ping 🙏

[elezar]

One issue with this is that as the temperature struct is extended with additional INPUT parameters, we would need to add additional arguments to this function. In other cases we explicitly allow the full struct to be passed in (see #159), but this was not a versioned struct.

If we were not dealing with a versioned struct, we would probably want to change this signature to (*Temperature) and only enure that we set the version in each of the handlers.

[dschervov]

I am grateful for for the full explanation! I agree about inconsistent API 💯

What should we do than? Maybe we can create V2 function, with this profile: func (handler TemperatureHandler) V2(sensorType TemperatureSensors) (Temperature, Return);

This will make sure that all V1 funciton calls in user code will be correctly executed, and the people who familiar with C nvml will be using V2 where they explicitly say what kind of sensor they want to read.

Later then the new sensor will be added (for memory for example), we already have the interface (V2 function) to use it. What do you think?

[elezar]

The V2 function can only be added once we have an nvmlTemperature_v2_t type defined -- which is not yet the case. For now, I think we may rather want something like:

diff --git a/pkg/nvml/device.go b/pkg/nvml/device.go
index 1cea1f9..7e378a3 100644
--- a/pkg/nvml/device.go
+++ b/pkg/nvml/device.go
@@ -3275,14 +3275,14 @@ func (device nvmlDevice) GetCoolerInfo() (CoolerInfo, Return) {
 
 // nvml.DeviceGetTemperatureV()
 type TemperatureHandler struct {
-	device nvmlDevice
-	sensorType TemperatureSensors
+	device           nvmlDevice
+	temperatureInput *Temperature
 }
 
 func (handler TemperatureHandler) V1() (Temperature, Return) {
 	var temperature Temperature
 	temperature.Version = STRUCT_VERSION(temperature, 1)
-	temperature.SensorType = uint32(handler.sensorType)
+	temperature.SensorType = handler.temperatureInput.SensorType
 	ret := nvmlDeviceGetTemperatureV(handler.device, &temperature)
 	return temperature, ret
 }
@@ -3291,8 +3291,8 @@ func (l *library) DeviceGetTemperatureV(device Device, sensorType TemperatureSen
 	return device.GetTemperatureV(sensorType)
 }
 
-func (device nvmlDevice) GetTemperatureV(sensorType TemperatureSensors) TemperatureHandler {
-	return TemperatureHandler{device, sensorType}
+func (device nvmlDevice) GetTemperatureV(temperature *Temperature) TemperatureHandler {
+	return TemperatureHandler{device, temperature}
 }
 
 // nvml.DeviceGetMarginTemperature()

but I'm still not convinced that this is correct / the best solution. @klueska what are your thoughts?

[dschervov]

Oh yeah, i get it now! Thats cool! 💪

[klueska]

I don't think there should be any parameters to the V() function. Any version specific parameters should be passed in the V1(), V2(), etc. methods.

Reagrding passing temperature *Temperature -- in the Go API we have tried to avoid passing the whole struct as input, and instead pass the individual INPUT parts of the struct as parameters instead. Is there a reason to break from that here?

[dschervov]

IIs there a reason to break from that here

I think right now its not that necessarily, because the TemperatureSensor can only have one value = 0 (GPU), and it is used when we create struct by default in Go.

We can introduce V2 function with additional parameter: TemperatureSensor, when NVML api will make another sensor type (for example for MEMORY).

[elezar]

@klueska this is a similar problem to what we had for the attestation report parameter for DeviceGetConfComputeGpuAttestationReport (see #159). As was the case there, for the original NVML API, the struct includes INPUT and OUTPUT fields. The additional complication here is that the function uses a VERSIONED struct. Our solution there was to explicitly allow users to pass in a reference to the struct to allow the input fields to be set.

@dschervov we don't control the version of the struct in the BINDINGS. These are determined by the NVML definitions that we are wrapping. I would also only expect a new struct version when we add new fields to the struct, and not when we add support for new values.