Skip to content
Open
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 70 additions & 71 deletions unsloth_zoo/llama_cpp.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import json
from tqdm.auto import tqdm as ProgressBar
from functools import lru_cache
import getpass
import inspect
import contextlib
import importlib.util
Expand Down Expand Up @@ -264,11 +265,11 @@ def install_package(package, sudo = False, print_output = False, print_outputs =
# Choose package manager based on system type
if system_type == "rpm":
pkg_manager = "yum" if os.path.exists('/usr/bin/yum') else "dnf"
install_cmd = f"{'sudo ' if sudo else ''}{pkg_manager} install {package} -y"
install_cmd = f"{'sudo -S ' if sudo else ''}{pkg_manager} install {package} -y"
elif system_type == "arch":
install_cmd = f"{'sudo ' if sudo else ''}pacman -S --noconfirm {package}"
install_cmd = f"{'sudo -S ' if sudo else ''}pacman -S --noconfirm {package}"
else: # Default to debian/apt-get
install_cmd = f"{'sudo ' if sudo else ''}apt-get install {package} -y"
install_cmd = f"{'sudo -S ' if sudo else ''}apt-get install {package} -y"

print(f"Unsloth: Installing packages: {package}")
if not (IS_COLAB_ENVIRONMENT or IS_KAGGLE_ENVIRONMENT):
Expand All @@ -278,11 +279,18 @@ def install_package(package, sudo = False, print_output = False, print_outputs =
f"Unsloth: Execution of `{install_cmd}` was cancelled!\n"\
"Please install llama.cpp manually via https://docs.unsloth.ai/basics/troubleshooting-and-faqs#how-do-i-manually-save-to-gguf"
)
with subprocess.Popen(install_cmd, shell = True, stdout = subprocess.PIPE, stderr = subprocess.STDOUT) as sp:
if sudo:
password = getpass.getpass(f"Enter password for user {getpass.getuser()}: ")
Comment on lines +282 to +283

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip prompting when passwordless sudo already works

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 👍 / 👎.

with subprocess.Popen(install_cmd, shell = True, stdin = subprocess.PIPE, stdout = subprocess.PIPE, stderr = subprocess.STDOUT) as sp:
if sudo and sp.stdin:
sp.stdin.write(f"{password}\n".encode())
sp.stdin.flush()
sp.stdin.close()

for line in sp.stdout:
line = line.decode("utf-8", errors = "replace").rstrip()

if "Permission denied" in line or "not open lock file" in line or "are you root?" in line or "fatal" in line:
if "Permission denied" in line or "not open lock file" in line or "are you root?" in line or "fatal" in line or "requires superuser" in line:
sp.terminate()
raise RuntimeError(f"[FAIL] Unsloth: Permission denied when installing package {package}\n"\
"This operation requires elevated sudo/root permissions. Please manually install missing packages and retry again"
Expand Down Expand Up @@ -313,7 +321,7 @@ def do_we_need_sudo(system_type="debian"):
# Choose update command based on system type
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"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not run full RPM upgrades during sudo probing

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 👍 / 👎.

elif system_type == "arch":
update_cmd = "pacman -Sy"
else:
Expand All @@ -323,7 +331,7 @@ def do_we_need_sudo(system_type="debian"):
with subprocess.Popen(update_cmd, shell = True, stdout = subprocess.PIPE, stderr = subprocess.STDOUT) as sp:
for line in sp.stdout:
line = line.decode("utf-8", errors = "replace").rstrip()
if "Permission denied" in line or "not open lock file" in line or "are you root?" in line or "fatal" in line:
if "Permission denied" in line or "not open lock file" in line or "are you root?" in line or "fatal" in line or "requires superuser" in line:
sp.terminate()
sudo = True
break
Expand Down Expand Up @@ -802,75 +810,66 @@ def install_llama_cpp(
build_errors.append(f"Windows cmake build failed: {str(e)}")

else:
# Linux/macOS: Try make first, then cmake
# Linux/macOS: Use cmake to build (build using make is already removed from llama.cpp)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try:
if print_output: print("Trying to build with make...")
try_execute("make clean", cwd = llama_cpp_folder, **kwargs)
try_execute(f"make all -j{cpu_count}", cwd = llama_cpp_folder, **kwargs)
build_success = True
print("Successfully built with make")
except Exception as e:
build_errors.append(f"Make failed: {str(e)}")
if print_output: print(f"Make failed, trying cmake...")
# Use cmake instead
try:
# Clean up any partial build
try_execute(f"rm -rf build", cwd = llama_cpp_folder, **kwargs)

# Build cmake configure command with library detection
cmake_configure = (
f"cmake . -B build "
f"-DBUILD_SHARED_LIBS=OFF -DGGML_CUDA={gpu_support}"
)

# Detect OpenMP library path (fixes GOMP linker errors)
gomp_path = _find_lib_path('libgomp.so')
if gomp_path:
cmake_configure += (
f" -DOpenMP_C_LIB_NAMES=gomp"
f" -DOpenMP_CXX_LIB_NAMES=gomp"
f" -DOpenMP_gomp_LIBRARY={gomp_path}"
)
# Clean up any partial build
try_execute(f"rm -rf build", cwd = llama_cpp_folder, **kwargs)

# Detect OpenSSL library paths
ssl_path = _find_lib_path('libssl.so')
crypto_path = _find_lib_path('libcrypto.so')
if ssl_path and crypto_path:
cmake_configure += (
f" -DOPENSSL_ROOT_DIR=/usr"
f" -DOPENSSL_SSL_LIBRARY={ssl_path}"
f" -DOPENSSL_CRYPTO_LIBRARY={crypto_path}"
)
# Build cmake configure command with library detection
cmake_configure = (
f"cmake . -B build "
f"-DBUILD_SHARED_LIBS=OFF -DGGML_CUDA={gpu_support}"
)

# LLAMA_CURL is deprecated upstream (ggml-org/llama.cpp#18791),
# so we pass ignore_deprecation=True to handle any deprecation warnings.
try_execute(
cmake_configure,
cwd = llama_cpp_folder,
ignore_deprecation = True,
**kwargs
)
try_execute(
f"cmake --build build --config Release "\
f"-j{cpu_count} --clean-first --target "\
f"{' '.join(llama_cpp_targets)}",
cwd = llama_cpp_folder,
**kwargs
# Detect OpenMP library path (fixes GOMP linker errors)
gomp_path = _find_lib_path('libgomp.so')
if gomp_path:
cmake_configure += (
f" -DOpenMP_C_LIB_NAMES=gomp"
f" -DOpenMP_CXX_LIB_NAMES=gomp"
f" -DOpenMP_gomp_LIBRARY={gomp_path}"
)
# Move compiled objects to main folder.
# Remove only the target binaries first to avoid
# "same file" errors when symlinks point into build/bin/.
try_execute(
"rm -f " + " ".join(llama_cpp_targets) + " && cp build/bin/llama-* .",
cwd = llama_cpp_folder,
**kwargs

# Detect OpenSSL library paths
ssl_path = _find_lib_path('libssl.so')
crypto_path = _find_lib_path('libcrypto.so')
if ssl_path and crypto_path:
cmake_configure += (
f" -DOPENSSL_ROOT_DIR=/usr"
f" -DOPENSSL_SSL_LIBRARY={ssl_path}"
f" -DOPENSSL_CRYPTO_LIBRARY={crypto_path}"
)
build_success = True
# Remove build folder
try_execute(f"rm -rf build", cwd = llama_cpp_folder, **kwargs)
if print_output: print("Successfully built with cmake")
except Exception as e:
build_errors.append(f"CMake failed: {str(e)}")

# LLAMA_CURL is deprecated upstream (ggml-org/llama.cpp#18791),
# so we pass ignore_deprecation=True to handle any deprecation warnings.
try_execute(
cmake_configure,
cwd = llama_cpp_folder,
ignore_deprecation = True,
**kwargs
)
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
)
Comment on lines +851 to +858

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
            )

# Move compiled objects to main folder.
# Remove only the target binaries first to avoid
# "same file" errors when symlinks point into build/bin/.
try_execute(
"rm -f " + " ".join(llama_cpp_targets) + " && cp build/bin/llama-* .",
cwd = llama_cpp_folder,
**kwargs
)
Comment on lines +862 to +866

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
)

build_success = True
# Remove build folder
try_execute(f"rm -rf build", cwd = llama_cpp_folder, **kwargs)
if print_output: print("Successfully built with cmake")
except Exception as e:
build_errors.append(f"CMake failed: {str(e)}")

if not build_success:
error_msg = "=== Unsloth: FAILED building llama.cpp ===\n"
Expand Down
Loading