Skip to content

Fix GGUF converter sibling imports#661

Merged
danielhanchen merged 1 commit into
unslothai:mainfrom
alkinun:fix-gguf-converter-sibling-imports
May 17, 2026
Merged

Fix GGUF converter sibling imports#661
danielhanchen merged 1 commit into
unslothai:mainfrom
alkinun:fix-gguf-converter-sibling-imports

Conversation

@alkinun

@alkinun alkinun commented May 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #5495

Summary

  • Add the cached converter script directory to sys.path before executing it.
  • Cover sibling imports such as llama.cpp conversion.py with a focused regression test.

Root cause

llama.cpp convert_hf_to_gguf.py imports sibling helper modules by bare name. Unsloth caches the script under ~/.unsloth/llama.cpp and executes it by path, but that directory was not on sys.path, so Kaggle could fail with ModuleNotFoundError: No module named conversion.

Validation

  • pytest tests/test_llama_cpp_loader.py

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request updates _load_module_from_path in unsloth_zoo/llama_cpp.py to include the script's directory in sys.path, allowing for the resolution of sibling imports. A new test case was added to verify this behavior. Feedback suggests that modifying sys.path globally can cause side effects and recommends restoring the original path after execution in both the library code and the test suite to prevent environment pollution.

Comment thread unsloth_zoo/llama_cpp.py
Comment thread tests/test_llama_cpp_loader.py Outdated
@alkinun alkinun force-pushed the fix-gguf-converter-sibling-imports branch from 07079b2 to 49d8d01 Compare May 17, 2026 05:39
@alkinun

alkinun commented May 17, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the review feedback by restoring sys.path after executing the cached converter module. The regression test now also snapshots/restores sys.path and asserts the temporary converter directory is not left in sys.path after the load completes.

Validation: pytest tests/test_llama_cpp_loader.py

@alkinun alkinun marked this pull request as ready for review May 17, 2026 06:02
@danielhanchen

Copy link
Copy Markdown
Member

Thanks this works!

@danielhanchen danielhanchen merged commit d50b31a into unslothai:main May 17, 2026
Brishen pushed a commit to Brishen/unsloth-zoo that referenced this pull request May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants