fix: strip trailing slashes from model paths at entrypoint#41
Conversation
…er, add error tests
|
Warning Review limit reached
More reviews will be available in 51 minutes and 21 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
While this PR is titled as a fix for trailing slashes, it introduces significant scope creep by adding remote GGUF support and refactoring UI logic. These additions have caused a substantial increase in complexity, particularly in src/modelinfo/parsers/huggingface.py (+41) and src/modelinfo/cli.py (+3), without corresponding documentation in the PR description.
A critical logic gap exists in the path-stripping implementation which fails to account for Windows backslashes, potentially leaving the original bug unresolved for local Windows users. Additionally, while the UI now correctly respects the --gpu-util parameter, this functional change is undocumented and lacks automated test verification.
About this PR
- This PR contains significant scope creep. It adds support for remote GGUF repositories and alters UI hardware-fit logic, which are not mentioned in the PR title or description. Large, undocumented feature additions increase the risk of regressions and should ideally be separated into distinct pull requests.
- The change in UI hardware-fit logic (switching from a hardcoded 90% utilization threshold to the dynamic
--gpu-utilparameter) is undocumented in the PR description. Please update the documentation to reflect how this parameter now influences UI warnings.
2 comments outside of the diff
src/modelinfo/cli.py
line 276🟡 MEDIUM RISK
Suggestion: Useos.path.basenameto correctly extract the model name regardless of the operating system's path separator. Relying onsplit('/')[-1]will fail to provide a clean display name on Windows systems using backslashes.
line 133🟡 MEDIUM RISK
Theanalyze_modelfunction is performing too many distinct tasks: determining if a path is remote, fetching from the network, and dispatching to format-specific parsers. Consider refactoring the resource resolution and loading logic into a dedicated helper function (e.g.,load_model_resources).
Test suggestions
- CLI strips trailing slashes from both single and multiple input paths
- Identify and fetch metadata for a remote GGUF repository
- Inspect a specific GGUF file within a remote repository using path-style notation
- Render a comparison table for GGUF repositories with multiple variants
- Hardware fit logic correctly utilizes the 'gpu_util' parameter for VRAM calculations
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Hardware fit logic correctly utilizes the 'gpu_util' parameter for VRAM calculations
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
|
||
| # Strip trailing slashes from paths/repos to prevent empty basenames and routing issues | ||
| if args.file: | ||
| args.file = [path.rstrip("/") for path in args.file if path] |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The current implementation only strips forward slashes. On Windows, users often use backslashes in local paths, which will still cause issues with basename extraction and directory checks if left unstripped. Consider using .rstrip("/\\") to handle both forward and backward trailing slashes.
| def fake_fetch(repo_id, *, fetch_tensors, timeout): | ||
| tensors = { | ||
| "__metadata__": { | ||
| "general.architecture": "llama", | ||
| "llama.block_count": 32, | ||
| "llama.attention.head_count_kv": 8, | ||
| "llama.attention.key_length": 128, | ||
| "gguf_variants": [ | ||
| {"filename": "model-q4.gguf", "size": 1000000000}, | ||
| {"filename": "model-q8.gguf", "size": 2000000000} | ||
| ], | ||
| "repo_id": "org/model-gguf" | ||
| } | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The tensors metadata dictionary defined here is redundant with the data returned by the _get_mock_gguf_group_data helper. Simplify the test by calling the helper directly.
| {"rfilename": "model-q4.gguf", "size": 1000000000} | ||
| ] | ||
| }).encode("utf-8") | ||
| elif "model-q4.gguf" in url: | ||
| import struct | ||
| header = b"GGUF" + struct.pack("<IQQ", 2, 0, 0) | ||
| return header | ||
| raise ValueError(f"Unexpected url: {url}") | ||
|
|
||
| monkeypatch.setattr(huggingface, "_make_request", fake_make_request) | ||
|
|
||
| tensors, config, format_name, disk_size = huggingface.fetch_huggingface_repo("org/model-gguf") | ||
|
|
||
| assert format_name == "GGUF" |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The mock implementation for Hugging Face API requests is duplicated across multiple GGUF parsing tests. Extracting this logic into a shared helper or a pytest fixture would improve maintainability.
| if gpu_name: | ||
| utilization = vram_bytes / (max_vram_gb * 1024**3) if max_vram_gb > 0 else 2.0 | ||
| if utilization <= 0.90: | ||
| if utilization <= gpu_util: |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: This correctly replaces the hardcoded 0.90 threshold with the user-configurable 'gpu_util' parameter as specified in the CLI arguments. Note that this change currently lacks an automated test scenario.
…, and test helper
1 participant
Summary
This fixes a bug where model paths with trailing slashes caused empty basenames in the comparison table. The change strips trailing slashes from the inputs in
args.fileat the entry point of the CLI.Motivation & Context
When running comparisons, trailing slashes on paths (for example,
meta-llama/Llama-2-7b-hf/) resulted insplit("/")[-1]returning an empty string. Stripping trailing slashes inmainprevents empty name columns and potential Hugging Face routing errors.Type of Change
How Has This Been Tested?
I added a unit test
test_cli_strips_trailing_slashes_from_model_pathsintests/test_cli.pyto verify that the CLI strips trailing slashes from model paths for both single and multiple model runs.Screenshots (if appropriate)
Checklist