Fix missing GPM metrics

[elezar]

These changes ensure that the collected GPM metrics are transferred to the original data structure passed to GpmMetricsGet.

Closes #121

[klueska]

Can we avoid doing a defer like this and do something more explicit?

[elezar]

Yes, I can store the incoming ret.

I was just wondering whether we should copy the metrics that are returned regardless of the ret value.

[klueska]

I'm trying to remember why we pass a pointer to the GpmMetricsGetType in here (and fill it out) rather than just returning (GpmMetricsGetType, Return). I have a vague memory of needing to do this but I don't know why.

[klueska]

I remember now -- because the API requires some fields to be set as INPUTs and other fields to be set as OUTPUTs in the struct: https://github.com/NVIDIA/go-nvml/blob/main/gen/nvml/nvml.h#L9401-L9409

[elezar]

As a matter of interest, if some fields are used as inputs, using the GpmMetricsGetV type seems quite akward. I suppose this is generated due to the versioned union though.

[klueska]

I don't like the idea of adding a one-off stub like this. If we are going to do this, we should do it for everything.

[elezar]

To be clear, I added one like this for the topology functions that I also added tests for. I didn't see the value in adding all of them if we weren't going to use them immediately.

Would you rather I remove the tests and the stubs?

[klueska]

I guess it's fine then. I think we can add a level of indirection pretty easily though. We just instruct nvidia.yml to rewrite all generated functions as nvml*Internal and then do something similar to how I generate the various interfaces to generate variables for all nvml* = nvml*Internal by default. Even better would be to make them all function pointers hanging off of the library type (so we don't have to do the weird update/reset in the tests), but that might be more trouble than it's worth.

[elezar]

Sure. Let's have a look at this at some point in the (near) future.

[klueska]

Does this need to be updated somehow too? Otherwise, how do the assigned metrics make their way out of this call?

[elezar]

The .convert() call does also copy the metrics meaning that any metrics in metricsGet will be in the result of convert().

[klueska]

I think we should change this whole thing:

// nvml.GpmMetricsGet()
type GpmMetricsGetVType struct {
	metricsGet *nvmlGpmMetricsGetType
}

func (l *library) GpmMetricsGetV(metricsGet *GpmMetricsGetType) GpmMetricsGetVType {
	return GpmMetricsGetVType{metricsGet.convert()}
}
func (metricsGetV GpmMetricsGetVType) V1() Return {
	metricsGetV.metricsGet.Version = 1
	return nvmlGpmMetricsGet(metricsGetV.metricsGet)
}

func (l *library) GpmMetricsGet(metricsGet *GpmMetricsGetType) Return {
	metricsGet.Version = GPM_METRICS_GET_VERSION
	return nvmlGpmMetricsGet(metricsGet.convert())
}

to this:

// nvml.GpmMetricsGet()
type GpmMetricsGetVType struct {
	metricsGet *GpmMetricsGetType
}

func (l *library) GpmMetricsGetV(metricsGet *GpmMetricsGetType) GpmMetricsGetVType {
	return GpmMetricsGetVType{metricsGet}
}
func (metricsGetV GpmMetricsGetVType) V1() Return {
	nvmlMetricsGet := metricsGetV.metricsGet.convert()
	nvmlMetricsGet.Version = 1
	ret := nvmlGpmMetricsGet(&nvmlMetricsGet)
	*metricsGetV.metricsGet = nvmlMetricsGet.convert()
	return ret
}

func (l *library) GpmMetricsGet(metricsGet *GpmMetricsGetType) Return {
	nvmlMetricsGet := metricsGet.convert()
	nvmlMetricsGet.Version = GPM_METRICS_GET_VERSION
	ret := nvmlGpmMetricsGet(&nvmlMetricsGet)
	*metricsGetV.metricsGet = nvmlMetricsGet.convert()
	return ret
}

[klueska]

I see how your most recent changes will work for GpmMetricsGet() (even though I still think we should use an explicit convert) -- but I still don't understand how you believe they work for GpmMetricsGetV(&metricsGet).V1() without something like what I have above.

[elezar]

Yes, you're right about V1(). I've updated the revision to add a test for that too and also update the implementation.

[klueska]

The .convert() call does also copy the metrics meaning that any metrics in metricsGet will be in the result of convert().

Yes, but we need the values set back in the original variable passed in.

[klueska]

remove?

[elezar]

Yes. Sorry, was editing in the wrong window here.

[elezar]

@klueska we create a local instance of an nvml type before the function call and we set the return value explicitly here.

[klueska]

OK, something changed since I made my previous comment. I was still seeing:

func (l *library) GpmMetricsGetV(metricsGet *GpmMetricsGetType) GpmMetricsGetVType {
	return GpmMetricsGetVType{metricsGet.convert()}
}

whereby we lose our reference to the original metricsGet variable as soon as we do metricsGet.convert(). The current code seems to basically do what I was suggesting now.

The one difference being that you do:

metricsGet.Metrics = nvmlMetricsGet.Metrics

instead of:

*metricsGet = nvmlMetricsGet.convert()

This is equivalent for now -- but your version would need to be updated if any other fields become OUT fields in the future, wheras using convert() avoids this.

[elezar]

@klueska following your comments, I could update this whole thing to:

Suggested change

	func gpmMetricsGet(metricsGet *GpmMetricsGetType) Return {
	nvmlMetricsGet := metricsGet.convert()
	ret := nvmlGpmMetricsGetStub(nvmlMetricsGet)
	metricsGet.Metrics = nvmlMetricsGet.Metrics
	return ret
	func gpmMetricsGet(metricsGet *GpmMetricsGetType) Return {
	nvmlMetricsGet := metricsGet.convert()
	ret := nvmlGpmMetricsGet(nvmlMetricsGet)
	*metricsGet = *nvmlMetricsGet.convert()
	return ret

do you have a preference?

[klueska]

This is equivalent for now -- but your version would need to be updated if any other fields become OUT fields in the future, wheras using convert() avoids this.

[elezar]

Ok. Let's switch to the full assignmet to better handle future fields.