Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow manually loading SD library #19

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

drasticactions
Copy link
Contributor

First, thank you for binding this! It saved me time setting it up myself ;).

For loading the stable-diffusion library, you have a nifty DllImport hijack to redirect it to search for the right library to load. That's great if you're using your libraries, but I built libstable-diffusion for Mac Catalyst. That will throw an exception since it doesn't count in .NET as running on MacOS (Since it's kinda iOS, kinda MacOS, it's a thing) and your code only checks if it's Windows, Mac, or Linux. Likewise, if you build stable-diffusion.cpp for iOS or Android, while your binding should work, it will blow up because of the DllImport code.

If you allow someone to manually pick the library to load, that would let a "power user" like me deal with the native assemblies myself. My PR should enable that, while keeping your existing system in place for others.

@DarthAffe
Copy link
Owner

Hey, thanks for the contribution.

I have to admit, that I'm not a fan of exposing this on the StableDiffusionModel as this might cause people to think it has to be done (which normally should not be the case), but I agree with your argument, that this kind of custom usage is currently not possible.

I might change that in the future, but for now it's a good solution to handle all kind of edge cases

@drasticactions
Copy link
Contributor Author

drasticactions commented Aug 1, 2024

That's fair. FWIW I'm fine if you don't want this merged, I had to do this for the app I'm working on to function and figured I would give it to you if you wanted it, but coming up with a better approach is fine too.

IMO, I would make the "Native" library code injection "opt-out able" and let the default DLLImport code work on its own. If you have "stable-diffusion.dll", "libstable-diffusion.so" or "libstable-diffusion.dylib" in your output directory, it should automatically be picked up when you invoke any of the native APIs by .NET (You don't need to include "libstable-diffusion" for the library name, that automatically gets checked by .NET when assembly loading so leaving it as "stable-diffusion" should get you the "lib" prefix for free). That would also let me include my own libraries without having you try to load them yourself. And if a user has an issue with dealing... I mean, they built their own libraries, they should know what they're doing ;). And your default library loading code is there should they want to use the libraries you provide.

That, or maybe create a "Custom" Backend class so we can make our own "Backend" with custom parameters. Right now that wouldn't work I think because I don't think the Import code gets far enough for it to be invoked, but something around that could make a cleaner API to use Custom libraries.

@DarthAffe
Copy link
Owner

Opting out to use the default DLL-Import seems like a solution only in your specific case where you want to build against one specific lib. It is not viable for any cases where you need to select between multiple options (like cuda/rocm/cpu) at runtime and still want to include an option that's currently not supported.

For now I'll merge it like this as it allows to completely bypass the loading mechanism which allows to get around all restrictions.
The long-term solution will most likely be a less restrictive backend system.

Thanks again for the input :)

@DarthAffe DarthAffe merged commit c3a14c2 into DarthAffe:master Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants