feat: Add Windows native support and migrate llama.cpp to ~/.unsloth/#526
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements by adding native Windows support and centralizing the llama.cpp installation into the ~/.unsloth directory. The changes are comprehensive, touching upon platform-specific package installation, build processes, and path handling. While the implementation is largely solid, I've pointed out a couple of instances of code duplication for detecting Visual Studio and OpenSSL installations. Refactoring this duplicated logic would improve the code's maintainability. Overall, these are great additions that will improve the user experience on Windows and keep project directories cleaner.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd3eae73c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- C1: Add UNSLOTH_LLAMA_CPP_PATH env var override for custom llama.cpp location - C2: Remove unsafe auto-update (git pull on every invocation); use existing binaries if they work - C3: Add backward compatibility fallback for existing ./llama.cpp in CWD - C4: Add _is_safe_to_delete() validation before shutil.rmtree on corrupted repos - C5: Remove dead LLAMA_CURL exception handler; use ignore_deprecation=True - H1: Move os.makedirs(UNSLOTH_HOME) from import-time to install_llama_cpp() runtime - H2: Quote llama_cpp_folder in shell commands (shlex.quote) for paths with spaces - H3: Use os.path.normpath() for Windows path comparison in quantize_gguf - M1: Add IS_WINDOWS to __all__ (imported by save.py) - M3: Add Arch Linux pacman handler in install_package - H6: Add comment clarifying LLAMA_OPENSSL=ON is valid (common/CMakeLists.txt), remove dead -DLLAMA_CURL=OFF else branch
Review: Fixes appliedI've pushed fixes for the critical and high-priority issues identified in the review: Critical fixes
High-priority fixes
Medium-priority fixes
Testing
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7057b7e28f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if os.path.basename(real_path) == "llama.cpp": | ||
| return True |
There was a problem hiding this comment.
Limit safe-delete fallback to CWD llama.cpp path
The new safety helper treats any directory named llama.cpp as deletable, regardless of where it lives. In install_llama_cpp(), if a user passes a non-repo path like /mnt/data/llama.cpp, the integrity check will mark it "corrupted" and shutil.rmtree it, which can recursively delete unrelated user data; the fallback should be constrained to the explicit CWD compatibility path (or another strict allowlist).
Useful? React with 👍 / 👎.
The Unsloth compiler failed on Gemma3 because @auto_docstring(custom_intro="""...""") spans multiple lines. The line-by-line decorator stripping only removed the @auto_docstring line but left custom_intro="""..."""\n) behind, causing a SyntaxError (unmatched ')'). Two fixes: 1. Line-by-line stripping now tracks open parens and skips continuation lines 2. Regex stripping uses re.DOTALL so (.*?) matches across newlines
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed4a9a1b5e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if os.path.basename(real_path) == "llama.cpp": | ||
| return True |
There was a problem hiding this comment.
Restrict deletion to verified llama.cpp clones
install_llama_cpp() now deletes “corrupted” directories via shutil.rmtree, but _is_safe_to_delete() treats any path whose basename is llama.cpp as safe. If a caller passes a custom folder named llama.cpp that is not a clone (or points at the wrong directory), this code will recursively remove user data instead of failing safely; deletion should be gated on stronger signals (for example, repo markers under expected install roots) before removing anything.
Useful? React with 👍 / 👎.
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bc428c412
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if real_path.startswith(real_home + os.sep): | ||
| return True | ||
| # Safe if it's a dir named llama.cpp (CWD-relative fallback) | ||
| if os.path.basename(real_path) == "llama.cpp": |
There was a problem hiding this comment.
Limit repo auto-deletion to managed install paths
install_llama_cpp() now calls shutil.rmtree() for “corrupted” repos, but _is_safe_to_delete() treats any path with basename llama.cpp as safe. If a caller points llama_cpp_folder to a user-managed directory like /mnt/shared/llama.cpp and that folder is missing one expected subdirectory, this branch will recursively delete it, causing data loss outside ~/.unsloth. The safety predicate should only allow known managed locations (or an explicit, exact fallback path), not any matching basename.
Useful? React with 👍 / 👎.
Resolve conflict in compiler.py: take main's improved decorator stripping from PR #525 (STRIP_DECORATORS set, decorator_head_re regex, proper paren_depth tracking) while keeping our re.DOTALL regex safety net for multiline decorator args.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a42e29ce
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Main's regex [^\)]{0,} already matches newlines (any char except
closing paren), so it handles multiline @auto_docstring decorators
identically to .*? with re.DOTALL. Verified empirically: both
regexes produce identical output on all decorator patterns.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2bb795ddb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| is_installed, package_name = check_libcurl_dev() | ||
| if not is_installed: | ||
| missing.append(package_name) |
There was a problem hiding this comment.
Stop blocking Windows builds on missing OpenSSL
The Windows requirements path makes OpenSSL mandatory by appending it to missing when _find_openssl_root() fails, and install_llama_cpp() will then fail early if winget cannot install it. In the actual build flow, OpenSSL is treated as optional (it only adds -DLLAMA_OPENSSL=ON when found), so environments without OpenSSL or winget are blocked from CPU/GPU builds that otherwise could succeed. This should be a non-fatal optional dependency check rather than a hard requirement.
Useful? React with 👍 / 👎.
- Normalize UNSLOTH_LLAMA_CPP_SCRIPTS_DIR via abspath/expanduser/expandvars and switch the candidate check to os.path.isfile so a relative env var or a directory named convert_hf_to_gguf.py no longer corrupts the lru_cache key or surfaces as IsADirectoryError. - Return (path, mtime_ns, size) from the resolver so the lru_cache invalidates when a pinned local converter is updated in place. - Differentiate the patched output filename by a sha256 prefix of the cache key so distinct sources do not overwrite each other in LLAMA_CPP_DEFAULT_DIR while their cached metadata diverges. - Initialize text_archs and vision_archs alongside supported_types so introspection failures (e.g. text-only converters with no MMPROJ entry) produce a clean warning instead of an UnboundLocalError at frozenset(). - Use logger.info instead of print for the local-script notice so it follows the module's logging routing. - Branch error log and RuntimeError text on the actual source (local file vs network download) for clearer diagnostics. - Fix Github "report" comment typo to "repository". Blame-touched lines preserved in intent: - The error-logging lines whose blame points at 70f2ce7 ("[Part 1] Complete llama.cpp Integration Overhaul ... Multi-Modal Support (unslothai#302)") still log with exc_info=True and re-raise a RuntimeError chained from the original exception; only the source label is now accurate ("local file '...'" vs "network download") because the local-script branch of the same try block can also fail there. - The patched_filename construction whose blame points at b9daae8 ("feat: Add Windows native support and migrate llama.cpp to ~/.unsloth/ (unslothai#526)") still writes into LLAMA_CPP_DEFAULT_DIR using os.path.join with a Python-built filename; the lru_cache(2) introduced alongside the local-override feature would otherwise let two cache entries share one on-disk file, so the filename now carries a 12-char sha256 of the cache key whenever a local script is in play, leaving the network-only filename unchanged.
- check_llama_cpp now consults UNSLOTH_LLAMA_CPP_SCRIPTS_DIR before falling back to llama_cpp_folder, so the install-check matches what _download_convert_hf_to_gguf will actually use; the candidate probe also moves from os.path.exists to os.path.isfile so a directory named convert(-|_)hf_to_gguf.py is no longer accepted as a script. Error text now mentions both lookup locations.
- use_local_gguf gains an optional llama_cpp_dir argument; when omitted it prefers UNSLOTH_LLAMA_CPP_SCRIPTS_DIR's sibling gguf-py before falling back to LLAMA_CPP_DEFAULT_DIR, so a pinned llama.cpp checkout's matching gguf-py is loaded by the converter's `import gguf` instead of failing with ModuleNotFoundError.
- Revert the sha256-suffixed patched filename. _download_convert_hf_to_gguf_cached now writes to the stable LLAMA_CPP_DEFAULT_DIR/{name}.py in both local and network modes, restoring backwards compatibility with convert_to_gguf()'s default converter_location keyword and avoiding orphaned digest files when a pinned converter is updated in place. Severity downgraded P1 -> P2 by finding-verify with disposition REVERT_UNSUBSTANTIATED; the cross-source disk/cache mismatch the digest was guarding against is hypothetical because every in-tree caller (unsloth_repo/unsloth/save.py, studio export.py) passes the returned path explicitly. The hashlib import added for the digest is removed.
- Replace `Failed during {source}/introspection of original script` with `Failed during {source} (introspection of original script)` so a quoted file path inside source no longer reads as if /introspection were a path component.
Blame-touched lines preserved in intent:
- The `use_local_gguf` signature and docstring (lines 150-151) whose blame points at 70f2ce7 ("[Part 1] Complete llama.cpp Integration Overhaul ... Multi-Modal Support (unslothai#302)") still defines the same context manager with the same external contract; only an optional llama_cpp_dir keyword is added so callers passing nothing keep the prior behavior whenever UNSLOTH_LLAMA_CPP_SCRIPTS_DIR is unset.
- The `gguf_py_path = os.path.join(LLAMA_CPP_DEFAULT_DIR, "gguf-py")` assignment (line 155) whose blame points at b9daae8 ("feat: Add Windows native support and migrate llama.cpp to ~/.unsloth/ (unslothai#526)") still computes gguf_py_path via os.path.join under a llama.cpp directory; the only change is that the directory is selected from UNSLOTH_LLAMA_CPP_SCRIPTS_DIR when that env var points at a checkout containing a gguf-py/, falling back to LLAMA_CPP_DEFAULT_DIR otherwise, so the Windows native install layout is preserved.
- The `check_llama_cpp` converter-discovery block (the comment, the for-loop over [convert-hf-to-gguf.py, convert_hf_to_gguf.py], the os.path.join call, the candidate probe, the converter_location assignment, the break, and the trailing pass at lines 565-571) whose blame points at 70f2ce7 ("[Part 1] Complete llama.cpp Integration Overhaul ... Multi-Modal Support (unslothai#302)") is preserved as the fallback branch when UNSLOTH_LLAMA_CPP_SCRIPTS_DIR is unset; the only change there is os.path.exists -> os.path.isfile to match _resolve_local_convert_script and reject directories with the script's name. The wrapping `if local_convert_script is not None: ... else:` only adds a new short-circuit; the unchanged for-loop is the else branch.
- The "Failed to find converter script in {llama_cpp_folder}" RuntimeError (line 574) whose blame points at 70f2ce7 ("[Part 1] Complete llama.cpp Integration Overhaul ... Multi-Modal Support (unslothai#302)") still raises the same RuntimeError class under the same condition (converter_location is None); only the message string is extended to also mention UNSLOTH_LLAMA_CPP_SCRIPTS_DIR so a misconfigured user sees both lookup locations.
Summary
Adds cross-platform Windows support to
unsloth_zoo/llama_cpp.pyand centralizes the llama.cpp installation under~/.unsloth/llama.cppinstead of polluting the current working directory.Windows Native Support
IS_WINDOWSflag alongside existingIS_COLAB_ENVIRONMENT/IS_KAGGLE_ENVIRONMENTinstall_package()installs viawingeton Windows with per-package arguments aligned tosetup.ps1(Git, CMake, VS Build Tools with C++ workload, OpenSSL)check_linux_type()returns"windows",check_libcurl_dev()checks OpenSSL at standard install paths,check_build_requirements()usesshutil.which()+ filesystem scan for VS Build Toolscheck_llama_cpp()appends.exesuffix and searchesbuild/bin/Release/install_llama_cpp()uses cmake with Visual Studio generator on Windows (skipsmake), passes-DLLAMA_OPENSSL=ONif OpenSSL is found, usesshutil.rmtree()instead ofrm -rfquantize_gguf()remaps default binary path on Windows;convert_to_gguf()usesos.path.joinfor cross-platform pathsdo_we_need_sudo()returnsFalseon Windows~/.unsloth/Home DirectoryUNSLOTH_HOME = ~/.unsloth,LLAMA_CPP_DEFAULT_DIR = ~/.unsloth/llama.cpp~/.unsloth/is created at import time viaos.makedirsLLAMA_CPP_DEFAULT_DIR— callers can still override viallama_cpp_folderparameteruse_local_gguf,check_llama_cpp,install_llama_cpp,_download_convert_hf_to_gguf(3 spots),convert_to_gguf,quantize_ggufBackward Compatibility
if IS_WINDOWS:llama_cpp_folderparameters still accept custom paths