llmodel: dont load libs in static initialization

[apage43]

this fixes some issues that were being seen on installed windows builds of 2.5.0

setImplementationsSearchPath did not work if llmodel was initialized before it was called

• do not actually load impl dlls in the static initializer, wait until we're actually trying to load a model for the first time

• rescan on the next model load attempt if the search path has been changed

• only load dlls that actually might be model impl dlls, otherwise we pull all sorts of random junk into the process before it might expect to be

[cosmic-snow]

Tried it again in the following combination of configurations and so far I didn't see any problems like in v2.5.0:

• Inside Qt Creator (Run Without Deployment) and a locally prepared offline installer
• Debug build and RelWithDebInfo

[cebtenzzre]

s_impl_libs should be a function-local static.

If we're at all concerned about thread safety, we should use a function to compute s_impl_libs and initialize it on the same line as the declaration - C++ guarantees that the initializer will be run only once. Otherwise, we should at least put if (s_scanned) { return s_impl_libs; } at the top to reduce the indentation level.

[apage43]

moving the decl/init into the fn, but the whole thing here is that we can't actually compute it (do the dlopen, etc.) at static init time because the search path may not actually be set yet (in the GUI case at least it is set by a different static initializer in a different file and it sometimes worked if that one ran first, but it is not guaranteed that it will)

if we want to actually be threadsafe around this we can't have a global mutable search path

[cebtenzzre]

Static local variables are not initialized until the first time control passes through their declaration. See cppreference.

[manyoso]

Let's chat about this. From what I can tell, you're both right. It should be static local, but the problem remains if that function is called before the global search path is set?

[apage43]

Yeahh either way if the method to set the search path is used it needs to have the re-checking behavior if it's not clearly documented to be useless unless called very early

I am not really clear on how this had been being called before that but I definitely caught this code running and loading libs before the chat gui had changed the path from the default .

[cebtenzzre]

Why does the chat client call setImplementationsSearchPath in the constructor of LLM instead of main.cpp?

And I think it would be a good idea to guard this code with a mutex (including the setting of s_scanned to false) in case it is called from more than one thread.

[apage43]

hmm, yeah, all the globalInstance()s should also be deferred til first use so should get run in the order they are called, and at least spot-checking it it does set the search path before its read when running from the build tree - built an installer to check and that also remains the case - so its likely the bigger culprit here was it dlopening dlls it shouldn't and that check is probably doing more work here than anything else

[cebtenzzre]

For reference, here is a description of the issue this PR addresses:

with a model in the dir it fairly consistently reserves the ram for it and then crashes before the gui even opens

making an installer locally and installing it also causes this, but running the same binary out of the build tree works fine

gets as far as finishing deserializing chats but the gui never even opens.. sometimes it crashes immediately and sometimes it hangs and when it hangs all the deserializing chats messages show twice

[apage43]

removed unnecessary unique_ptr wrapping that was only needed when I was attempting to stage the Implementations collected in a scan in a separate vector and move them over to the main one at the end

[apage43]

#1556 fixes the issues I was encountering on its own, this (messing with the init order) probably isn't necessary