Fix llama.cpp build failure when saving model to GGUF#687
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the deprecated 'make' build path for Linux and macOS, transitioning the build process entirely to 'cmake' to align with upstream llama.cpp changes. The review feedback suggests optimizing the build commands by removing redundant flags and unnecessary line continuations, as well as improving robustness through target name quoting and consistent use of the 'ignore_deprecation' parameter.
| try_execute( | ||
| f"cmake --build build --config Release "\ | ||
| f"-j{cpu_count} --clean-first --target "\ | ||
| f"{' '.join(llama_cpp_targets)}", | ||
| cwd = llama_cpp_folder, | ||
| ignore_deprecation=True, | ||
| **kwargs | ||
| ) |
There was a problem hiding this comment.
The --clean-first flag is redundant here because the build directory is already explicitly removed and recreated at the start of this block (line 815). Additionally, the backslashes for line continuation are unnecessary within function call parentheses. For robustness, it is recommended to quote the target names using shlex.quote to handle potential special characters or spaces in custom target names.
try_execute(
f"cmake --build build --config Release -j{cpu_count} --target {' '.join(shlex.quote(t) for t in llama_cpp_targets)}",
cwd = llama_cpp_folder,
ignore_deprecation = True,
**kwargs
)| try_execute( | ||
| "rm -f " + " ".join(llama_cpp_targets) + " && cp build/bin/llama-* .", | ||
| cwd = llama_cpp_folder, | ||
| **kwargs | ||
| ) |
There was a problem hiding this comment.
To ensure consistency with the rest of the build process and avoid potential failures from unexpected deprecation warnings in system tools (as noted in the PR description), ignore_deprecation = True should be passed here as well. Additionally, quoting the targets and handling the case where llama_cpp_targets might be empty will make the command more robust against various environments and configurations.
| try_execute( | |
| "rm -f " + " ".join(llama_cpp_targets) + " && cp build/bin/llama-* .", | |
| cwd = llama_cpp_folder, | |
| **kwargs | |
| ) | |
| rm_cmd = f"rm -f {' '.join(shlex.quote(t) for t in llama_cpp_targets)} && " if llama_cpp_targets else "" | |
| try_execute( | |
| f"{rm_cmd}cp build/bin/llama-* .", | |
| cwd = llama_cpp_folder, | |
| ignore_deprecation = True, | |
| **kwargs | |
| ) |
|
|
||
| else: | ||
| # Linux/macOS: Try make first, then cmake | ||
| # Linux/macOS: Use cmake to build (build using make is already removed from llama.cpp) |
There was a problem hiding this comment.
We need to ensure that cmake dependency is installed already and is the right verion for this
I'm not entirely sure why we were preferring make over cmake previously @danielhanchen any idea?
There was a problem hiding this comment.
@Datta0, llama.cpp build indeed fails when cmake is not yet installed. I will try to improve the code for this PR to cater such scenario. There's already a check but it leads to error.
There was a problem hiding this comment.
Hi @Datta0 and @danielhanchen, I updated the implementation regarding the installation of system packages required to build llama.cpp from source. I updated the implementation of do_we_need_sudo() and install_package(). It can now ask for a password if sudo is required for the installation of the system packages.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45312364d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if system_type == "rpm": | ||
| pkg_manager = "yum" if os.path.exists('/usr/bin/yum') else "dnf" | ||
| update_cmd = f"{pkg_manager} check-update" | ||
| update_cmd = f"{pkg_manager} update -y" |
There was a problem hiding this comment.
On RPM-based systems this changes the permission/metadata probe from check-update to update -y, so install_llama_cpp() can now upgrade every installed system package before installing llama.cpp dependencies (immediately when running as root, or again through sudo {update_cmd} below). The DNF command reference documents update as an alias of upgrade, which updates packages, while check-update only checks for available updates, so this can unexpectedly mutate production hosts during a GGUF save.
Useful? React with 👍 / 👎.
| ) | ||
| if sudo: | ||
| password = getpass.getpass(f"Enter password for user {getpass.getuser()}: ") | ||
| install_cmd = f"echo {password} | {install_cmd}" |
There was a problem hiding this comment.
When sudo is required, interpolating the password into echo {password} | ... makes the secret part of the shell command line, which other local processes can observe, and passwords containing shell metacharacters can also alter or break the install command. Feed the password to sudo -S via stdin/communicate(input=...) instead of constructing a shell string with it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hi @Datta0, @danielhanchen, and @rolandtannous, I addressed this by using stdin.write() in providing sudo password.
… password if it requires sudo
harriebird
force-pushed
the
fix/llama-cpp-build-failure-when-converting-to-gguf
branch
from
4531236 to
b6c4eb3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6c4eb3e05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if sudo: | ||
| password = getpass.getpass(f"Enter password for user {getpass.getuser()}: ") |
There was a problem hiding this comment.
In passwordless-sudo environments such as CI or containers, do_we_need_sudo() can set sudo=True after the unprivileged update fails and then successfully run sudo {update_cmd} without a password before returning. This new unconditional getpass prompt makes package installation block or raise EOF before trying the sudo -S ... install command that would have succeeded without input, so GGUF setup can no longer auto-install missing packages in those environments.
Useful? React with 👍 / 👎.
2 participants
Building
llama.cppusingmakeis already replaced bycmake, which causes an error. Building viamakein theinstall_llama_cpp()was removed for Linux/macOS systems. For more information about this, here's the latestMakefilecontent:https://github.com/ggml-org/llama.cpp/blob/master/Makefile
Another issue is that deprecation warnings when calling
try_execute()raises anRuntimeErrorby default. This causes the building ofllama.cppfrom source to stop. To fix this,ignore_deprecationwas set toTruewhen callingtry_execute()to build thellama.cpp.This solves issue unslothai/unsloth#5832.
Screenshot for the llama.cpp build after cloning its repository:
Screenshot for the llama.cpp build using the existing cloned source code: