Allow path to libnvidia-ml.so.1 to be specified #81
Conversation
elezar
force-pushed
the
allow-path-to-be-set
branch
from
e0f6516 to
df745e9
Compare
elezar
force-pushed
the
allow-path-to-be-set
branch
6 times, most recently
from
dfa4760 to
aa0b8a7
Compare
| if libnvml.path == "" { | ||
| libnvml.path = defaultNvmlLibraryName | ||
| } | ||
| if libnvml.flags == 0 { |
There was a problem hiding this comment.
How do you unset all flags?
There was a problem hiding this comment.
Is that a use case that is expected? If so we should use a *int to detect whether it is set or not.
Note it's not currently possible to set the flags through the options, so we could address this in a follow-up.
There was a problem hiding this comment.
Note sure. I doubt anyone will ever override this (beyond adding to it) so its probably fine.
There was a problem hiding this comment.
Let's reevaluate if it ever becomes relevant.
| var newDynamicLibrary = func(name string, flags int) dynamicLibrary { | ||
| return dl.New(name, flags) | ||
| var newDynamicLibrary = func(path string, flags int) dynamicLibrary { | ||
| return dl.New(path, flags) |
There was a problem hiding this comment.
I guess the dl library is already capable of handling a full path, we just never used this until now?
There was a problem hiding this comment.
Yes. When dl.Open is called, the path is forwarded to C.dlopen which has well defined semantics for handling the path and where to search.
|
|
||
| // SetLibraryOptions applies the specified options to the NVML library. | ||
| // If this is called when a library is already loaded, and error is raised. | ||
| func SetLibraryOptions(opts ...LibraryOption) error { |
There was a problem hiding this comment.
I think it was in the last MR, but can you explain the purpose of the Default() method? I'm just thinking about it from a callsite perspective --- nvml.Default() doesn't really imply to me that I'm getting a reference to the default implementation of the nvml Interface. When / how do you actually expect this to be used?
There was a problem hiding this comment.
This returns a reference (as an interface) to the default global instance or libnvml. In go-nvlib we're using this as:
// TODO: For now we rely on the default NVML library and perform the lookups against this.
return nvml.Default().Library().Lookup(name)
Note that once we implement a New() call that returns an Interface instead this should not be required anymore and we would instantiate the library explicitly.
Can you think of a way to make this clearer?
There was a problem hiding this comment.
I see -- its specifically so we can get at that Library method -- can we just make a top level Library() function then?
There was a problem hiding this comment.
Would GetLibrary() be ok? We would either need to rename this or the Library interface.
I think a top-level function is fine as I would see something similar happening for the NVML functions.
There was a problem hiding this comment.
Hmm, right -- I'd rather the top level call be the same as what we would eventually call on an instance of Interface. Maybe that becomes GetLibrary() as well?
There was a problem hiding this comment.
OK. I renamed the function GetLibrary and also included an implementation at the package level.
elezar
force-pushed
the
allow-path-to-be-set
branch
from
aa0b8a7 to
0e2a408
Compare
This change adds a SetLibOptions function that allows the default library name of libnvidia-ml.so.1 to be overridden. This is useful on systems where the location of this library is known but this location cannot be relied upon to be in the LD_LIBRARY_PATH or ldcache. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
force-pushed
the
allow-path-to-be-set
branch
from
0e2a408 to
e756be6
Compare
These changes add a basic API for allowing the path to
libnvidia-ml.so.1to be set.This can be used in environments where the path to the library can be determined programatically but not reliably added to the
LD_LIBRARY_PATHbefore program startup, for example.This builds on the changes from #86