Implement model quantization format support

[j4ys0n]

Implemented comprehensive GGUF quantization support using llama.cpp, enabling conversion of HuggingFace models to GGUF format with multiple quantization levels (Q4_K_M, Q5_K_M, Q6_K, Q8_0, etc.).

Key changes:

• Added llama.cpp installation to Docker container for GGUF conversion
• Implemented GGUFQuantizer class with two-step quantization process:
  1. Convert HF model to intermediate GGUF format (f16/f32/q8_0)
  2. Quantize to target precision using llama-quantize
• Extended QuantizationConfig with GGUF-specific parameters:
  • gguf_quant_type: Target quantization level (Q4_K_M default)
  • gguf_intermediate_format: Intermediate format (f16 default)
• Added comprehensive input validation for GGUF quantization types
• Updated UI with GGUF selection and parameter controls
• Enhanced documentation with GGUF usage guidelines and recommendations
• Added error handling for llama.cpp availability checks
• Included subprocess-based streaming output for conversion progress

Quantization types supported:

• Q2_K, Q3_K_S, Q3_K_M, Q3_K_L (smaller sizes)
• Q4_0, Q4_1, Q4_K_S, Q4_K_M (balanced - Q4_K_M recommended)
• Q5_0, Q5_1, Q5_K_S, Q5_K_M (best quality)
• Q6_K, Q8_0 (high precision)
• F16, F32 (full precision)

GGUF outputs are compatible with llama.cpp, Ollama, and other GGUF-compatible inference engines.

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR introduces GGUF quantization support using llama.cpp, enabling conversion of HuggingFace models to GGUF format with multiple quantization levels. It adds a new quantization method to the UI, updates configuration handling, and implements a two-step process: first converting to an intermediate GGUF format, then quantizing to the target precision. The Docker image is updated to include llama.cpp dependencies.

Key Changes & Positives

• 🟢 Added GGUF quantization method with full UI integration and parameter controls
• 🟢 Implemented robust validation for GGUF quantization types and intermediate formats
• 🟢 Enhanced Dockerfile with llama.cpp installation and build process
• 🟢 Added comprehensive documentation updates for GGUF usage and recommendations
• 🟢 Included subprocess-based streaming output for conversion progress monitoring

Potential Issues & Recommendations

1. Issue / Risk: The llama-quantize availability check uses bash -lc which may not work reliably in all container environments.

   • Impact: Could cause false negatives in availability checks, preventing quantization from starting.
   • Recommendation: Simplify to direct which llama-quantize check or use shutil.which() for better portability.
   • Status: 🟡 Needs review

2. Issue / Risk: The intermediate file naming logic may cause conflicts if multiple quantization runs occur with same model ID.

   • Impact: Could overwrite previous intermediate files or create naming collisions.
   • Recommendation: Add timestamp or UUID to intermediate file names for uniqueness.
   • Status: 🟡 Needs review

3. Issue / Risk: The convert_hf_to_gguf.py path is hardcoded to /opt/llama.cpp/convert_hf_to_gguf.py.

   • Impact: May break if the build process changes the script location or if the path is different in other environments.
   • Recommendation: Make this configurable or dynamically detect the script location.
   • Status: 🟡 Needs review

4. Issue / Risk: The GGUFQuantizer._check_llama_cpp_available() method doesn't verify that the convert_hf_to_gguf.py script exists.

   • Impact: Could proceed with quantization even if the conversion script is missing.
   • Recommendation: Add explicit check for the existence of convert_hf_to_gguf.py script.
   • Status: 🟡 Needs review

Language/Framework Checks

• Python:
  • ✅ Proper use of subprocess with streaming output
  • ✅ Exception handling with meaningful error messages
  • ✅ Type checking and validation for GGUF parameters
  • � Minor: Consider using pathlib.Path for path operations instead of os.path.join
• Docker:
  • ✅ Correct installation of build dependencies (git, cmake, build-essential)
  • ✅ Proper cleanup of apt cache
  • � Minor: Could add --no-cache to git clone for reproducibility

Security & Privacy

• No security or privacy concerns identified in this diff.

Build/CI & Ops

• No specific build or ops concerns in this diff.

Tests

• No test changes included. Consider adding unit tests for:
  • GGUFQuantizer methods (availability check, conversion, quantization)
  • Configuration validation for GGUF parameters
  • Edge cases like invalid quantization types

Approval Recommendation

Approve with caveats

The implementation is well-structured and adds significant value with GGUF support. However, the following items should be addressed before merging:

• Verify llama-quantize availability check reliability
• Address intermediate file naming collision risks
• Ensure convert_hf_to_gguf.py path is configurable or detectable
• Add explicit check for conversion script existence

[Copilot]

Pull Request Overview

This PR adds GGUF quantization support to MSQuant, enabling users to quantize models using the llama.cpp toolchain. GGUF quantization produces models compatible with llama.cpp, Ollama, and other GGUF-compatible inference engines.

• Implements GGUFQuantizer class with support for multiple quantization levels (Q4_K_M, Q5_K_M, Q6_K, Q8_0, etc.)
• Adds GGUF-specific configuration parameters (gguf_quant_type, gguf_intermediate_format)
• Updates Docker build to install llama.cpp toolchain and dependencies

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/msquant/core/quantizer/engine.py	Adds GGUFQuantizer class with methods for model download, conversion to GGUF format, and quantization using llama.cpp tools
src/msquant/core/quantizer/config.py	Extends QuantizationConfig with GGUF parameters and validation for quantization types and intermediate formats
src/msquant/app/pages/configure.py	Updates UI to include GGUF as a quantization method option with selectors for quantization type and intermediate format
pixi.toml	Adds gguf package dependency
docker/Dockerfile.gpu	Installs llama.cpp from GitHub with build tools and adds it to PATH
README.md	Documents GGUF quantization support with recommended settings and usage notes

Comments suppressed due to low confidence (2)

src/msquant/core/quantizer/engine.py:510

• The generic vLLM instruction is misleading for GGUF outputs. GGUF models are not loaded via vLLM but through llama.cpp, Ollama, or other GGUF-compatible engines. This message should be conditional on the quantization method to provide accurate next steps.

        logger.info(f"Next step: load in vLLM, e.g., `vllm serve {output_dir} --dtype float16`")

src/msquant/core/quantizer/engine.py:1

• The new GGUFQuantizer class and its methods (_check_llama_cpp_available, _download_model, _convert_to_gguf_intermediate, _quantize_gguf, run) lack test coverage. Consider adding tests to verify the GGUF quantization workflow, parameter validation, and error handling.

"""

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The script name uses underscores (convert_hf_to_gguf.py) but llama.cpp's actual script uses hyphens (convert-hf-to-gguf.py). This will cause the conversion to fail at runtime when the script is not found.

Suggested change

	convert_script = "/opt/llama.cpp/convert_hf_to_gguf.py"
	convert_script = "/opt/llama.cpp/convert-hf-to-gguf.py"

[Copilot]

The uppercase conversion is applied regardless of whether it's needed. The validation at line 120 expects exact matches from the valid_gguf_types list (which contains uppercase values). If a non-string is passed and stored as-is, the validation could fail unexpectedly. Consider either requiring string type or handling non-string gracefully with a type check/conversion.

Suggested change

	self.gguf_quant_type = gguf_quant_type.upper() if isinstance(gguf_quant_type, str) else gguf_quant_type
	self.gguf_intermediate_format = gguf_intermediate_format.lower() if isinstance(gguf_intermediate_format, str) else gguf_intermediate_format
	if not isinstance(gguf_quant_type, str):
	raise ValueError(f"gguf_quant_type must be a string, got {type(gguf_quant_type).__name__}")
	self.gguf_quant_type = gguf_quant_type.upper()
	if not isinstance(gguf_intermediate_format, str):
	raise ValueError(f"gguf_intermediate_format must be a string, got {type(gguf_intermediate_format).__name__}")
	self.gguf_intermediate_format = gguf_intermediate_format.lower()

[Copilot]

Checking out the master branch of llama.cpp means the build will use whatever version is current at build time, leading to non-reproducible builds and potential breaking changes. Consider pinning to a specific commit hash or tag for reproducibility and stability.

Suggested change

	git checkout master && \
	git checkout 7e908c3 && \ # Replace 7e908c3 with the desired commit hash or tag

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR implements GGUF quantization support using llama.cpp, enabling conversion of HuggingFace models to GGUF format with multiple quantization levels. It adds GGUF-specific parameters to the quantization config, updates the UI with GGUF selection and controls, and includes comprehensive validation and error handling. The Dockerfile is updated to install llama.cpp dependencies.

Key Changes & Positives

• 🟢 Added GGUF quantization support with full quantization type validation
• 🟢 Implemented two-step quantization process (convert → quantize) using llama.cpp
• 🟢 Extended UI with GGUF-specific parameters and documentation
• 🟢 Added comprehensive input validation for GGUF quantization types
• 🟢 Updated Dockerfile to include llama.cpp installation
• 🟢 Added subprocess-based streaming output for conversion progress

Potential Issues & Recommendations

1. Issue / Risk: llama-quantize command availability check is incomplete
   Impact: Could fail silently if tools aren't properly installed
   Recommendation: Use shutil.which() or direct command execution with error handling instead of bash -lc wrapper
   Status: 🟡 Needs review

2. Issue / Risk: Hardcoded convert script path in _convert_to_gguf_intermediate
   Impact: May break if llama.cpp location changes
   Recommendation: Make path configurable or use environment variable
   Status: 🟡 Needs review

3. Issue / Risk: No timeout handling for subprocess operations
   Impact: Long-running processes could hang indefinitely
   Recommendation: Add timeout parameter to subprocess calls
   Status: 🔴 Problem

4. Issue / Risk: Insecure subprocess command construction
   Impact: Potential shell injection vulnerability
   Recommendation: Use subprocess.run() with list arguments instead of shell=True
   Status: 🔴 Problem

Language/Framework Checks

• Python:
  • ✅ Proper exception handling with RuntimeError and ValueError
  • ✅ Type hints and validation in config
  • ✅ Logging with QuantizationLogger
  • � Missing: Context manager usage for file operations
  • � Missing: Async/await patterns for long-running tasks

Security & Privacy

• 🔴 No explicit secrets handling in the diff
• � No sensitive data exposure in logs or config
• � Environment variables properly used for paths

Build/CI & Ops

• 🟢 Dockerfile includes llama.cpp installation with proper cleanup
• 🟢 Pixi.toml adds gguf dependency
• � No explicit CI/CD changes visible
• � No observability or monitoring configuration changes

Tests

• � No test additions visible in diff
• � Should include unit tests for:
  • GGUFQuantizer methods
  • Config validation
  • Error handling scenarios
  • Subprocess command execution

Approval Recommendation

Request changes

The implementation introduces significant new functionality but has critical security and reliability concerns:

1. Subprocess command construction is vulnerable to injection
2. No timeout handling for long-running operations
3. Incomplete tool availability checking
4. Hardcoded paths that may break in different environments

Additionally, comprehensive testing is missing. These issues must be addressed before merging. The GGUF support is valuable but requires robust implementation to be production-ready.

[j4ys0n]

Automated review 🤖

Summary of Changes

This PR introduces GGUF quantization support using llama.cpp, enabling conversion of HuggingFace models to GGUF format with multiple quantization levels. It adds a new quantization method to the UI, extends configuration with GGUF-specific parameters, and implements a two-step quantization process (conversion + quantization). The Docker image is updated to include llama.cpp binaries, and documentation is enhanced to reflect the new capabilities.

Key Changes & Positives

• 🟢 GGUF Support Added: New quantization method with full UI integration and parameter controls.
• 🟢 Docker Setup: llama.cpp binaries are now included in the GPU Docker image.
• 🟢 Input Validation: Comprehensive validation for GGUF quantization types and intermediate formats.
• 🟢 Two-Step Process: Clear separation of model conversion and quantization steps for GGUF.
• 🟢 Documentation Updates: README updated with GGUF usage guidelines and quantization type recommendations.

Potential Issues & Recommendations

1. Issue / Risk: which python check in _check_llama_cpp_available may not reliably detect Python in containerized environments.

   • Impact: Could cause false negatives in availability checks.
   • Recommendation: Replace with direct Python executable check or use shutil.which("python").
   • Status: 🟡 Needs review

2. Issue / Risk: Subprocess output streaming may not be fully robust for all llama.cpp outputs.

   • Impact: Progress logs might be incomplete or delayed.
   • Recommendation: Add error handling for stream reading and consider buffering improvements.
   • Status: 🟡 Needs review

3. Issue / Risk: Hardcoded path /opt/llama.cpp/convert-hf-to-gguf.py may not be portable.

   • Impact: Could break if installation location changes.
   • Recommendation: Use environment variable or dynamic path resolution.
   • Status: 🟡 Needs review

4. Issue / Risk: llama-quantize command assumes binary is in PATH without fallback.

   • Impact: May fail silently if binary is not found.
   • Recommendation: Add fallback logic or explicit error message with installation steps.
   • Status: 🟡 Needs review

Language/Framework Checks

• Python:
  • ✅ Type hints and validation in config
  • ✅ Proper exception handling with RuntimeError
  • ✅ Subprocess usage with streaming output
  • � subprocess.Popen usage is correct but could benefit from more robust error handling
• Docker:
  • ✅ Correct installation of llama.cpp binaries
  • ✅ PATH environment variable correctly set
  • ✅ Use of apt-get for dependencies
• TypeScript/Node: Not present in diff

Security & Privacy

• 🟢 No secrets or credentials in the diff
• 🟢 Input validation prevents injection-like issues
• 🟢 Model download uses HuggingFace Hub API securely

Build/CI & Ops

• 🟢 Dockerfile updates are appropriate for new dependencies
• 🟢 Pixi.toml includes gguf package
• 🟢 No breaking changes to existing build process

Tests

• 🔴 No test additions included in diff
• 🟡 Need unit tests for:
  • GGUFQuantizer methods (_check_llama_cpp_available, _convert_to_gguf_intermediate, _quantize_gguf)
  • Configuration validation for GGUF parameters
  • End-to-end quantization workflow
• 🟡 Consider integration tests for Docker environment

Approval Recommendation

Approve with caveats

The implementation is well-structured and adds significant value with GGUF support. However, the following items should be addressed before merging:

• Review subprocess output handling for robustness
• Verify hardcoded paths are portable
• Add unit tests for new quantizer logic
• Confirm availability check logic is reliable in containerized environments

[Copilot]

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

src/msquant/core/quantizer/engine.py:1

• The availability check for 'convert-hf-to-gguf.py' only verifies that Python is available, not the actual script. The comment on line 250 says 'Check if convert-hf-to-gguf.py is available', but the code checks for Python instead. This means the function will pass even if the conversion script is missing, causing later failures. Add a proper check for the convert-hf-to-gguf.py script using which convert-hf-to-gguf.py or by checking if the file exists at the expected path.

"""

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The hardcoded path '/opt/llama.cpp/convert-hf-to-gguf.py' assumes a specific installation location. If the llama.cpp installation path changes or the script is unavailable at this location, the conversion will fail. Consider making this configurable via an environment variable (e.g., LLAMA_CPP_PATH) or detecting the script location dynamically using which convert-hf-to-gguf.py or searching common installation paths.

[Copilot]

[nitpick] Using capture_output=True parameter is available, but the pattern is inconsistent with line 262 where both stdout=subprocess.PIPE and stderr=subprocess.STDOUT are used in subprocess.Popen calls. For consistency and clarity, consider using the same approach throughout the class. Note that capture_output=True is equivalent to stdout=subprocess.PIPE, stderr=subprocess.PIPE.

[Copilot]

The snapshot_download is missing error handling for common failure scenarios like network issues, authentication failures, or invalid model IDs. While there's a general exception catch on line 290, consider adding specific handling for HfHubHTTPError or RepositoryNotFoundError to provide more informative error messages to users about what went wrong.

[Copilot]

The cleanup logic on line 449 checks if intermediate_file != final_file, but when the quantization type matches the intermediate format (F16 or F32), line 358 uses shutil.copy2(input_file, output_file) which creates a copy. This means when gguf_quant_type and gguf_intermediate_format are the same (e.g., both 'F16'), the intermediate file will be unnecessarily deleted after being copied to the same-named final file. The condition should also check if the files have different names to avoid deleting files that were just copied.

Suggested change

	if intermediate_file != final_file and os.path.exists(intermediate_file):
	if os.path.abspath(intermediate_file) != os.path.abspath(final_file) and os.path.exists(intermediate_file):

[Copilot]

The LLAMA_CPP_VERSION value 'b6945' appears to be a commit hash or short version identifier, but it's not clear what full version this corresponds to or why this specific version was chosen. Consider adding a comment explaining the version selection rationale (e.g., 'Using b6945 for stability/compatibility with feature X') to help maintainers understand the version choice when updating dependencies.

Suggested change

	# Install llama.cpp for GGUF quantization support
	# Install llama.cpp for GGUF quantization support
	# Using commit b6945 for stability and compatibility with GGUF quantization as of June 2024.

[Copilot]

Using sanitize=False when rendering HTML disables XSS protection. While the current content is static and safe, this pattern creates a security risk if the content is ever changed to include dynamic data. Consider using sanitize=True (the default) or restructuring to use NiceGUI's native components instead of raw HTML.

Suggested change

	''', sanitize=False)
	''')