Fix llama.cpp quantize location and execution on Windows. #2894
Conversation
unsloth/save.py
Outdated
| possible_paths = [ | ||
| os.path.join(".", "llama.cpp", "quantize.exe"), | ||
| os.path.join(".", "llama.cpp", "quantize"), | ||
| os.path.join(".", "llama.cpp", "llama-quantize.exe"), | ||
| os.path.join(".", "llama.cpp", "llama-quantize"), | ||
| os.path.join(".", "llama.cpp", "build", "bin", "llama-quantize"), | ||
| os.path.join(".", "llama.cpp", "build", "bin", "quantize"), | ||
| os.path.join(".", "llama.cpp", "build", "bin", "Release", "llama-quantize.exe"), | ||
| os.path.join(".", "llama.cpp", "build", "bin", "Release", "quantize.exe") | ||
| ] | ||
|
|
||
| for path in possible_paths: | ||
| if os.path.exists(path): | ||
| quantize_location = path | ||
| break |
There was a problem hiding this comment.
Can we wrap this in a get_llamacpp_binary_location function or something and use the same thing in https://github.com/unslothai/unsloth/pull/2894/files#diff-46849d25980ee8d9337f4f8c30369faf36ceda3479272fd737ebf5ad9c703840R887-R897 ?
There was a problem hiding this comment.
Should be resolved. Confirmed successful execution with update + failure case by deleting the binaries.
I think the current approach is optimal: we don't expect the file to be in other locations than these specified, and no need to waste time trawling the filesystem if it's determinate, especially since it's really only 3 paths with 2 different executable names. |
|
Hey @Datta0 anything else needed here to get this PR in? |
2 participants
This PR fixes two specific issues with gguf quantization on Windows:
./prefix to the located quantization executable rather than relying on os.path functions to refer to the file properly. This bug produced a consistent exception on Windows:'.' is not recognized as an internal or external command, operable program or batch file.. I have modified the quantize_location variable to use the os library and remove the prefix, and the issue was resolved for me in local testing.llama.cpp/build/bin/Releasepath, rather than thellama.cpp/build/bin. This is a smaller consideration, as I wasn't able to get llama.cpp compilation working via the script anyway on Windows due to curl linking issues, but I figured I might as well add proper path detection based on the cmake outputs I was seeing. It would take significantly more work to fix the overall automatic compilation flow, but this at least saves a step of manual effort.Closes: #1645
Can also be closed as no longer needed: #1646