Conversation
This commit addresses four critical issues: 1. Docker build failure: Made llama.cpp extraction more robust by automatically detecting the extracted directory name instead of hardcoding it. This prevents failures when the directory structure doesn't match expectations. 2. Security: Removed sanitize=False from ui.html() in configure.py. The HTML content is purely static with no user input, so sanitization can be safely enabled to prevent potential XSS risks. 3. GGUF cleanup logic: Improved intermediate file cleanup to properly check if files are the same using os.path.samefile(), preventing issues on case-insensitive filesystems and avoiding deletion of files that shouldn't be removed. 4. Error handling: Added comprehensive error handling for HuggingFace model downloads with specific error messages for common failure scenarios (authentication, gated repos, network issues, etc.). Files changed: - docker/Dockerfile.gpu: Robust llama.cpp extraction - src/msquant/app/pages/configure.py: Remove sanitize=False - src/msquant/core/quantizer/engine.py: Improve cleanup logic and error handling
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling, file system handling, and Docker installation robustness for the GGUF quantization functionality.
- Enhanced error handling in
_download_model()with specific exception types and user-friendly error messages - Fixed GGUF quantization logic to properly handle case-insensitive filesystems and format matching
- Improved Docker installation script to handle potential variations in llama.cpp directory structure
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/msquant/core/quantizer/engine.py | Added detailed exception handling for model downloads, improved format comparison logic, and fixed cleanup logic for case-insensitive filesystems |
| src/msquant/app/pages/configure.py | Removed deprecated sanitize=False parameter from ui.html() call |
| docker/Dockerfile.gpu | Enhanced llama.cpp installation to be more robust against directory name variations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import shutil | ||
| shutil.copy2(input_file, output_file) | ||
| # Skip quantization if target format matches intermediate format | ||
| if quant_type.upper() == intermediate_format.upper(): |
There was a problem hiding this comment.
This comparison may fail when intermediate_format could have values like 'q8_0' (lowercase with underscore) but quant_type could be 'Q8_0' (uppercase). The comparison should normalize the format strings by converting to a consistent case and handling underscores and hyphens uniformly, or by checking both uppercase and lowercase variants.
| except (OSError, ValueError): | ||
| # If samefile fails, fall back to string comparison | ||
| if intermediate_file != final_file and os.path.exists(intermediate_file): | ||
| logger.info(f"Cleaning up intermediate file: {intermediate_file}") | ||
| os.remove(intermediate_file) |
There was a problem hiding this comment.
[nitpick] The fallback logic duplicates the cleanup code from lines 500-501. Consider extracting the cleanup logic into a helper function or restructuring to avoid duplication.
| rm /tmp/llama.zip && \ | ||
| unzip -q /tmp/llama.zip -d /tmp/llama-extract && \ | ||
| EXTRACTED_DIR=$(find /tmp/llama-extract -maxdepth 1 -type d -name "llama-*" | head -1) && \ | ||
| if [ -z "$EXTRACTED_DIR" ]; then echo "Error: No llama directory found after extraction"; ls -la /tmp/llama-extract; exit 1; fi && \ |
There was a problem hiding this comment.
[nitpick] The error handling combines multiple operations on a single line with &&, making it harder to read and debug. Consider splitting into multiple RUN commands or separate lines within the same RUN for better clarity.
This commit addresses typecheck failures and improves GGUF format comparison: 1. Format comparison normalization: Added _normalize_format() method to properly compare GGUF format strings (e.g., 'q8_0' vs 'Q8_0') by converting to uppercase and normalizing separators (hyphens to underscores). This prevents unnecessary quantization when formats are equivalent. 2. Fixed imports: Changed imports from huggingface_hub.utils to huggingface_hub.errors as per the correct module structure. 3. Exception handling order: Reordered exception handlers to catch more specific exceptions (GatedRepoError, LocalEntryNotFoundError) before their base classes to avoid unreachable code warnings. 4. Explicit sanitize parameter: Added explicit sanitize=True to ui.html() call to satisfy type checker requirements. All typecheck errors are now resolved.
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
The sanitize parameter only accepts False or a function, not True. Removed the parameter entirely to use the default sanitization behavior, which is safer than explicitly passing False. Type signature: sanitize: ((str) -> str) | Literal[False]
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if e.response.status_code == 401: | ||
| raise RuntimeError( | ||
| f"Authentication failed for model '{model_id}'. " | ||
| f"Please log in with 'huggingface-cli login'." | ||
| ) from e | ||
| elif e.response.status_code == 403: | ||
| raise RuntimeError( | ||
| f"Access denied for model '{model_id}'. " | ||
| f"You may need to accept the model's license agreement on HuggingFace Hub." | ||
| ) from e | ||
| else: |
There was a problem hiding this comment.
Potential AttributeError if HfHubHTTPError doesn't have a response attribute or if response is None. Add a guard check: if hasattr(e, 'response') and e.response and e.response.status_code == 401:
| if e.response.status_code == 401: | |
| raise RuntimeError( | |
| f"Authentication failed for model '{model_id}'. " | |
| f"Please log in with 'huggingface-cli login'." | |
| ) from e | |
| elif e.response.status_code == 403: | |
| raise RuntimeError( | |
| f"Access denied for model '{model_id}'. " | |
| f"You may need to accept the model's license agreement on HuggingFace Hub." | |
| ) from e | |
| else: | |
| if hasattr(e, 'response') and e.response and e.response.status_code == 401: | |
| raise RuntimeError( | |
| f"Authentication failed for model '{model_id}'. " | |
| f"Please log in with 'huggingface-cli login'." | |
| ) from e | |
| elif hasattr(e, 'response') and e.response and e.response.status_code == 403: | |
| raise RuntimeError( | |
| f"Access denied for model '{model_id}'. " | |
| f"You may need to accept the model's license agreement on HuggingFace Hub." | |
| ) from e | |
| elif hasattr(e, 'response') and e.response and hasattr(e.response, 'status_code'): |
| if os.path.exists(intermediate_file) and os.path.exists(final_file): | ||
| if not os.path.samefile(intermediate_file, final_file): | ||
| logger.info(f"Cleaning up intermediate file: {intermediate_file}") | ||
| os.remove(intermediate_file) |
There was a problem hiding this comment.
[nitpick] The nested condition on line 507 can be combined with line 506 for better readability: if os.path.exists(intermediate_file) and os.path.exists(final_file) and not os.path.samefile(intermediate_file, final_file):
| if os.path.exists(intermediate_file) and os.path.exists(final_file): | |
| if not os.path.samefile(intermediate_file, final_file): | |
| logger.info(f"Cleaning up intermediate file: {intermediate_file}") | |
| os.remove(intermediate_file) | |
| if os.path.exists(intermediate_file) and os.path.exists(final_file) and not os.path.samefile(intermediate_file, final_file): | |
| logger.info(f"Cleaning up intermediate file: {intermediate_file}") | |
| os.remove(intermediate_file) |
The sanitize parameter is required by NiceGUI's ui.html() method. Setting sanitize=False is safe here because: - The HTML content is a static string literal in source code - No user input or dynamic content is interpolated - Only uses standard safe HTML tags (<p>, <strong>, <br>) This fixes the typecheck error: "Argument missing for parameter 'sanitize'"
|
Automated review 🤖 Summary of Changes Key Changes & Positives
Potential Issues & Recommendations
Language/Framework Checks
Security & Privacy
Build/CI & Ops
Tests
Approval Recommendation
|
j4ys0n
deleted the
claude/fix-github-action-docker-011CUoPeTPiWiNZ1Qebmrj4R
branch
This commit addresses four critical issues:
Docker build failure: Made llama.cpp extraction more robust by automatically detecting the extracted directory name instead of hardcoding it. This prevents failures when the directory structure doesn't match expectations.
Security: Removed sanitize=False from ui.html() in configure.py. The HTML content is purely static with no user input, so sanitization can be safely enabled to prevent potential XSS risks.
GGUF cleanup logic: Improved intermediate file cleanup to properly check if files are the same using os.path.samefile(), preventing issues on case-insensitive filesystems and avoiding deletion of files that shouldn't be removed.
Error handling: Added comprehensive error handling for HuggingFace model downloads with specific error messages for common failure scenarios (authentication, gated repos, network issues, etc.).
Files changed: