Introduces new plug-in manager infrastructure #1068
Conversation
Add capability to add new plug-ins types Signed-off-by: Aleksandr Bilkovskii <[email protected]>
|
👋 Hi alexanderbilk! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
| * @param plugin_name Name of the plugin to load | ||
| * @return Typed plugin handle or nullptr if not found or cast fails | ||
| */ | ||
| template<typename handleType = basePluginHandle> |
There was a problem hiding this comment.
Why do we need to template all these methods? Is there no other way?
| * Derived classes override to provide specific version checking logic | ||
| */ | ||
| virtual bool | ||
| checkApiVersion(void *plugin_interface) const = 0; |
There was a problem hiding this comment.
Why use void * instead of proper types?
| struct pluginConfig { | ||
| std::string initFuncName; // e.g., "nixl_plugin_init" | ||
| std::string finiFuncName; // e.g., "nixl_plugin_fini" | ||
| std::string filenamePrefix; // e.g., "libplugin_" |
| std::string initFuncName; // e.g., "nixl_plugin_init" | ||
| std::string finiFuncName; // e.g., "nixl_plugin_fini" | ||
| std::string filenamePrefix; // e.g., "libplugin_" | ||
| std::string filenameSuffix; // e.g., ".so" |
There was a problem hiding this comment.
Why is ".so" configurable?
| std::string finiFuncName; // e.g., "nixl_plugin_fini" | ||
| std::string filenamePrefix; // e.g., "libplugin_" | ||
| std::string filenameSuffix; // e.g., ".so" | ||
| int expectedApiVersion; // Expected API version for validation |
There was a problem hiding this comment.
This is from the API of the backend plugins, I don't think it should belong here
| int expectedApiVersion; // Expected API version for validation | ||
| }; | ||
|
|
||
| template<typename pluginInterface> struct success_result { |
There was a problem hiding this comment.
Overall code comment.
This implementation has fundamental architectural issues.
It tries to use three distinct and conflicting C++ paradigms simultaneously:
- C-style code: void* + static casts
- Classic OOP: Inheritance (base vs derived), virtual functions.
- Template metaprogramming
This is a red flag, since a subset of these (possibly even just one) would be sufficient to implement the functionality.
The typing is strange. There appears to be type-safety by offering templated methods like loadPlugin, but implementation-wise, it is completely unsafe. The internal logic immediately type-erases the pointer to void*, passes it around effectively untyped, and then performs a hard static_cast or reinterpret_cast later.
This is much worse than the code we currently have, which is using proper typing for the object returned by init().
The ownership model is confusing.
The base class thinks it owns the plugin handles (loadedPlugins_ map).
The templates think the caller defines the plugin type ().
The derived class thinks it defines the validation logic (checkApiVersion) but it is shared with the base class.
I find this code very hard to read and follow.
I think the amount of work needed to fix this is much higher than implementing a dispatcher:
Extend the existing nixlPluginManager to hold two maps (loaded_backend_p_ and loaded_telemetry_p_) and use symbol probing (dlsym) to identify which map a loaded library belongs to.
| void *plugin_interface) override; | ||
|
|
||
| bool | ||
| canUnloadPlugin(const std::string &plugin_name) const override; |
There was a problem hiding this comment.
Please consider using std::string_view when API is just read-only, without storing param
| bool | ||
| checkApiVersion(void *plugin_interface) const override; | ||
|
|
||
| std::shared_ptr<basePluginHandle> |
There was a problem hiding this comment.
why shared pointer?
Typically unique_ptr is sufficient for factory method
| info.name = name; | ||
| info.createFunc = creator; | ||
| static_plugins_.push_back(info); | ||
| staticPlugins_.push_back(info); |
There was a problem hiding this comment.
- can use emplace_back
- keeping const char * inside info will not work for dynamic strings. Maybe keep as std::string?
| virtual const char * | ||
| getVersion() const = 0; | ||
|
|
||
| const void * |
There was a problem hiding this comment.
opaque type maybe not be the best option in C++
|
|
||
| protected: | ||
| mutable std::mutex lock_; | ||
| std::map<std::string, std::shared_ptr<basePluginHandle>, std::less<>> loadedPlugins_; |
There was a problem hiding this comment.
maybe unique_ptr?
std::less is redundant? std::map is already an ordered collection
There was a problem hiding this comment.
The std::less<> is not redundant, the default is std::less< std::string > which does not allow searching the map with a different, but compatible, type, e.g. with a std::string_view.
| const void *plugin_interface) | ||
| : dlHandle_(std::move(handle)), | ||
| pluginInterface_(plugin_interface) { | ||
| assert(dlHandle_ && "DlHandleDeleter must not be null"); |
|
|
||
| basePluginManager::basePluginManager(pluginConfig config) : config_(std::move(config)) {} | ||
|
|
||
| plugin_load_result<void> |
There was a problem hiding this comment.
instead of using failure result - just throw an exception, this is not pref critical path
This will greatly simplify the design, not need to use variant
| * limitations under the License. | ||
| */ | ||
|
|
||
| #ifndef __BASE_PLUGIN_MANAGER_H |
There was a problem hiding this comment.
Suggest NIXL_SRC_CORE_BASE_PLUGIN_MANAGER_H.
4 participants
Add capability to add new plug-ins types