[VL] Promote SharedLibraryLoader to SPI#10774
Conversation
zhztheplayer
left a comment
zhztheplayer
left a comment
There was a problem hiding this comment.
Thanks. May I know the motivation of adding the SPI? Do you place a user-customized shared loader implementation?
| val loaders = ServiceLoader | ||
| .load(classOf[SharedLibraryLoader]) | ||
| .asScala | ||
| .filter(loader => loader.accepts(osName, osVersion)) |
There was a problem hiding this comment.
What happens when more than one loaders are returned?
There was a problem hiding this comment.
First in, first load && Best effort to load one if successful.
There was a problem hiding this comment.
Do we already have this use case (multiple loaders returned)? Otherwise let's simply throw an error because the loading priority is not guaranteed?
There was a problem hiding this comment.
This is new. If we leave this conflict error for users, how do they suppose to fix it
There was a problem hiding this comment.
For example, if user adds their own loader implementation of CentOS 9, does the code ensure their implementation is chosen over the built-in SharedLibraryLoaderCentos9?
Can we add a simple priority API to SharedLibraryLoader, so the loading process is more deterministic?
There was a problem hiding this comment.
For example, if user adds their own loader implementation of CentOS 9, does the code ensure their implementation is chosen over the built-in SharedLibraryLoaderCentos9?
The new SharedLibraryLoaderCentos9 can define its own accepts logic with osName or osVersion to distinguish the built-in one. Then, configs loadLibOS/loadLibOSVersion can help the caller side to decide which one to pick.
Can we add a simple priority API to SharedLibraryLoader, so the loading process is more deterministic?
I tried, but the priority API didn't mitigate the issue here since the conflict loaders can also have identical priority.
Maybe we shall just throw errors when conflicting
There was a problem hiding this comment.
Making sense to me then. Thanks for the explanation.
Yes, and then I don't need to modify the gluten main code base |
|
Thank you, @zhztheplayer, for the detailed review and suggestions. |
|
No problem. Thank you for the contribution! |
2 participants
What changes are proposed in this pull request?
This PR promotes SharedLibraryLoader to SPI. Enable developers to create a custom SharedLibraryLoader tailored to their specific environments.
Besides the existing
loadLibfunction, a new methodacceptsis defined forSharedLibraryLoaders to identify whether it satisfies the given options or os release information.How was this patch tested?
new tests