update to latest stable-diffusion.cpp #2
Conversation
src/NodeModule.cpp
Outdated
| @@ -277,6 +277,7 @@ namespace | |||
| Napi::PropertyDescriptor::Value("Default", Napi::Number::New(env, DEFAULT)), | |||
| Napi::PropertyDescriptor::Value("Discrete", Napi::Number::New(env, DISCRETE)), | |||
| Napi::PropertyDescriptor::Value("Karras", Napi::Number::New(env, KARRAS)), | |||
| Napi::PropertyDescriptor::Value("GITS", Napi::Number::New(env, GITS)), | |||
There was a problem hiding this comment.
tiny nitpick, but the order of these is different from the types file
| @@ -8,13 +8,16 @@ declare module "@lmagder/node-stable-diffusion-cpp" { | |||
| DPMPP2M, | |||
| DPMPP2Mv2, | |||
| LCM, | |||
| IPNDM, | |||
There was a problem hiding this comment.
Need to add these new values to sampleMethodEnum.DefineProperties() also in the C++ code
There was a problem hiding this comment.
Oops missed that, thanks for catching.
| @@ -30,6 +30,19 @@ FetchContent_Declare( | |||
| GIT_TAG f9ff166845a59327eda431af82ee85a9c7532c5d | |||
| ) | |||
|
|
|||
| if(NOT WIN32) | |||
| if(EXISTS ${CMAKE_SYSROOT}/usr/lib/x86_64-linux-gnu/libvulkan.so) | |||
There was a problem hiding this comment.
Is there a reason to not try find_package(Vulkan) first and only do this if !Vulkan_FOUND or does find_package find the wrong thing? I know I had some weird issues on GH actions runners with CUDA.
| @@ -336,7 +337,11 @@ namespace | |||
| { | |||
| Napi::Value tmp; | |||
| const auto params = info[0].ToObject(); | |||
| const auto model = params.Get("model").ToString().Utf8Value(); | |||
| const auto model = (tmp = params.Get("model"), tmp.IsUndefined() ? "" : tmp.ToString().Utf8Value()); | |||
There was a problem hiding this comment.
Is it valid to pass a "" model to new_sd_ctx? I assumed no, which was why this was previously not an optional param but maybe I'm wrong?
There was a problem hiding this comment.
It became optional in the flux PR here https://github.com/leejet/stable-diffusion.cpp/pull/356/files#diff-a8e875e8f7684a2f9e35212d88ee9574001952eb2c049385e73d5e24fd6e2a83L172-R205 - for flux inference need to set the diffusionModel instead of model.
| set(SD_FLASH_ATTN ON) | ||
| set(SD_VULKAN ON) | ||
| endif() | ||
|
|
||
| set(IMPLIB_SOURCE_FILES "") | ||
| if (CUDAToolkit_FOUND) | ||
| set(SD_CUBLAS ON) |
There was a problem hiding this comment.
do we need to check !Vulkan_FOUND to disable the CUDA paths here? I don't think one binary can support both in GGML currently.
Same for all the if (CUDAToolkit_FOUND)s
There was a problem hiding this comment.
You're right its either/or. I updated it like you suggested, so it will build with Vulkan if its installed and disable CUDA in that case.
…oesnt error if cuda_version.json is an empty object
|
Thanks for the updates! |
|
Very welcome, thanks for building and publishing the bindings! I was looking into how we could ship a prebuild for Vulkan possibly, is that something you would like to add? |
|
Yeah I think I would need to rejig the build scripts a bit, but it seems like a good idea. I want to test this a bit before making a new release so I'll take a look |
|
Ah and note that this commit from your stable-diffusion.cpp fork is missing in this PR, because I switched to leejet/stable-diffusion.cpp for testing. Also, while checking out stable-diffusion.cpp's backend loading code I noticed that flash attention is only used for CPU backends atm. So it might make sense to switch the |
I updated stable-diffusion.cpp to be able to play with flux and stable-diffusion 3.5.
Changes:
IPNDMandIPNDM_Vsampling methods to enumguidanceparam to txt2img and img2imgGITSscheduling to enumdiffusionModel,clipL,clipGandt5xxlparams to context creation params