Skip to content

fix: incorrect loading of native library with multiple backends #594

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

Closed
wants to merge 2 commits into from

Conversation

AsakusaRinne
Copy link
Collaborator

@AsakusaRinne AsakusaRinne commented Mar 12, 2024

What this PR does

It fixes the incorrect loading of native library when having multiple backends installed. This issue has been mentioned in #456 and #589.

Description about the issue

The behavior in master branch now is quite wierd.

  1. When using LLama.Example project directly, all things go as exepected.
  2. When installing both cpu and cuda backend packages and using LLamaSharp in another project, it cannot load correct library even if the log does show it loads the desired library.

Let's consider the case2. I had many tries and it's concluded as below.

  • When I used WithCuda or WithLibrary to load the cuda version library, the log shown it had selected that file to load but actually no gpu is used. Besides I could confirm it's not because of the model loading problem.
  • There were 5 folders/files in my output path, which are llama.dll, avx, avx2, avx512, cuda11. When I was trying to load the cuda library, I could delete neither avx or cuda11 folder. That means, these two files were both occupied by a certain process, which is obviously the one I started.
  • If I deleted avx folder, the case became that avx2 and cuda11 were both occupied. Still, cuda library could not be used.
  • I confirmed the order of searching was correct and I didn't see a double-loading in logs.

After deleting all the folders except cuda11, I found it works. The only thing I could come up with is that avx, avx2, etc. hit the keywords of dotnet runtime and it does not obey the rules we write in our code.

Finally I changed all the paths and insert one more level directory llama-sharp into the paths of native libraries. All things worked as expected again on my PC.

To be honest I don't think my solution is good enough because I still don't know the reason behind it. Besides, through this debug I found some problems in current native loading code, so I'd like to take this PR as a discussion before releasing next version with LLaVA.

@martindevans @SignalRT Hope that this mention doesn't bother you. Your participation will help a lot. :)


Discussion

NativeLibraryConfig with LLaVA

I think we just need to use the same way with llama.dll to deal with LLaVA here. If I'm not mistaking it, there was already a discussion about whether to put llava libraries in each backend packages. I'm not sure if it still needs discussion.

OpenCL support

It seems that OpenCL related logic has not been added to NativeLibraryConfig. I'll add it later if it's not ignored intentionally.

More flexible loading strategy

I think maybe we should make a better abstraction and allow user to override some APIs to add more flexible control of library loading. Currently, as far as I know, users who want to publish an app with LLamaSharp often maintain the location of native libraries themselves, while their user could have various environments. Finally it always fallbacks to using WithLibrary, limiting the usage of our control strategy.

However the time cost of it seems to be a problem. I think it should at least be no hurry.

@AsakusaRinne AsakusaRinne added bug Something isn't working break change backend labels Mar 12, 2024
@AsakusaRinne
Copy link
Collaborator Author

AsakusaRinne commented Mar 12, 2024

cc @vvdb-architecture @adammikulis @xuzeyu91 Would you like to help to test on this PR? Here's some steps for reference:

  1. clone the code and checkout to this PR.
  2. build the LLamaSharp project.
  3. Go to LLama/packages and find the nuget package generated in step 2.
  4. In Visual Studio, add the path in step3 as a nuget package folder and select it as the source.
  5. (important) delete C:\Users\xxx.nuget\packages\llamasharp\0.10.0 to clear the cache.
  6. Install your self-compiled LLamaSharp package.
  7. a) Follow .github/prepare_release.sh to compile backends and install them. or b) Install published backend packages but modify the content of your output path every time you rebuild your project.

@martindevans
Copy link
Member

If you're unable to delete those two folders my best guess would be that we're explicitly loading one library (e.g. CUDA) but then it's also implicitly loading the AVX library when one of the native methods is called.

Maybe we should experiment with using SetDllImportResolver (link? There's been discussion about doing that before, and it's really the "proper" way to do this I think.

@AsakusaRinne
Copy link
Collaborator Author

AsakusaRinne commented Mar 12, 2024

Yes, there must be two libraries loaded at the same time.

I've tried SetDllImportResolver but it seemed to only show the library symbol name (llama) instead of library file path when calling a native api.

@martindevans
Copy link
Member

I've tried SetDllImportResolver but it seemed to only show the library symbol name (llama) instead of library file path when calling a native api.

What I meant about using SetDllImportResolver is rather than using our current system (which loads the DLL ahead of time, and then just hopes that dotnet uses it) we could instead do all of our loading logic inside SetDllImportResolver. It's the proper API for it, and if we do that we'll know exactly what the loading system is doing because we're in control of it!

@SignalRT
Copy link
Collaborator

NativeLibraryConfig with LLaVA

I think we just need to use the same way with llama.dll to deal with LLaVA here. If I'm not mistaking it, there was already a discussion about whether to put llava libraries in each backend packages. I'm not sure if it still needs discussion.

I agree that we need the same way to load both libraries, llama.dll and llava_share.dll. My approach was to use the same directory layout, so I would suggest keeping this path and adapt the directory structure to the one that works better to the project.

The only thing that we didn´t discuss yet was if llava libraries will be in the same nuget packages (that's my preference, it will be the easiest way to the user).

@adammikulis
Copy link

@AsakusaRinne did you still want me to try this change out or should I wait while you all explore SetDllImportResolver as the solution?

@SignalRT SignalRT mentioned this pull request Mar 13, 2024
4 tasks
@AsakusaRinne
Copy link
Collaborator Author

@AsakusaRinne did you still want me to try this change out or should I wait while you all explore SetDllImportResolver as the solution?

It's okay to try it later, no hurry please

@martindevans
Copy link
Member

martindevans commented Mar 14, 2024

I've started experimenting with a SetDllImportResolver based approach, just as a prototype for testing. I'll push it up as a separate PR.

Edit: Here it is: #603

@AsakusaRinne
Copy link
Collaborator Author

After some test, I believe #603 is a better approach to resolve this problem and it'll be completed in another PR. Close this now, thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend break change bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants