search

NVIDIA / go-nvml

Go Bindings for the NVIDIA Management Library (NVML)
Apache License 2.0
290 stars 62 forks source link

Wrap all opaque types as interfaces #112

Closed klueska closed 5 months ago

klueska commented 5 months ago

Initial change look good. The main question I have is whether to top-level library functions that operate ON other types make sense, or whether we should always use the functions off the types?

I'd be fine returning concrete types for everything (including the top level library) and moving the API definitions to the testing package. I went back and forth on this and don't really have a preference, so long as we have a way to pull in a mock version of the library (and its other types) from somewhere.

elezar commented 5 months ago

Initial change look good. The main question I have is whether to top-level library functions that operate ON other types make sense, or whether we should always use the functions off the types?

No, that's not what I meant -- although it is a valid question. I meant that we have, as an example, three functions that do the same thing:

DeviceGetComputeMode(device Device)  (ComputeMode, Return)
library.DeviceGetComputeMode(device Device) (ComputeMode, Return)
Device.GetComputeMode()  (ComputeMode, Return)

My question was whether we would consider moving towards requiring the use of Device.GetComputeMode() and at least deprecate the other usages? This would drastically reduce the size of the top-level Interface.

klueska commented 5 months ago

Initial change look good. The main question I have is whether to top-level library functions that operate ON other types make sense, or whether we should always use the functions off the types?

No, that's not what I meant -- although it is a valid question. I meant that we have, as an example, three functions that do the same thing:

DeviceGetComputeMode(device Device)  (ComputeMode, Return)
library.DeviceGetComputeMode(device Device) (ComputeMode, Return)
Device.GetComputeMode()  (ComputeMode, Return)

My question was whether we would consider moving towards requiring the use of Device.GetComputeMode() and at least deprecate the other usages? This would drastically reduce the size of the top-level Interface.

I would be inclined to keep them since that is what matches the actual NVML API. That said, we should add tooling to autogenerate the top level Interface as well as the package level aliases from the functions hanging off the library type.