Feat/cli timeout flag#32
Open
John-1-1-1 wants to merge 5 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (6)
WalkthroughAdds a ChangesConfigurable HF network timeout
IDE configuration setup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_hf_timeout_configurable.py (2)
7-17: ⚡ Quick winAssert that CLI actually forwards
--timeoutinto the fetch call.This test currently proves parsing/execution succeeds, but it does not verify the main→analyze→fetch timeout handoff.
Suggested patch
try: result = main(["--timeout", "20.0", "gpt2"]) assert result == 0 + mock_fetch.assert_called_once() + assert mock_fetch.call_args.kwargs.get("timeout") == 20.0 except SystemExit as e: pytest.fail(f"CLI failed with SystemExit: {e}. --timeout argument is likely not implemented.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_hf_timeout_configurable.py` around lines 7 - 17, The test_cli_timeout_argument function currently only verifies that the CLI execution succeeds, but does not actually verify that the --timeout argument value (20.0) is forwarded to the mocked fetch_huggingface_repo function. After the main() call completes successfully, add an assertion on mock_fetch to verify it was called with the timeout parameter set to 20.0. This ensures the timeout value flows through the entire call chain from CLI parsing through to the actual fetch function call.
44-45: ⚡ Quick winValidate timeout on every mocked
urlopeninvocation, not just the final one.Using
mock_urlopen.call_argschecks only the last call, so partial propagation bugs can slip through.Suggested patch
- args, kwargs = mock_urlopen.call_args - assert kwargs.get('timeout') == 20.0, f"Expected timeout=20.0, but got {kwargs.get('timeout')}" + assert mock_urlopen.call_count >= 1 + for _, kwargs in mock_urlopen.call_args_list: + assert kwargs.get("timeout") == 20.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_hf_timeout_configurable.py` around lines 44 - 45, The current assertion using mock_urlopen.call_args only validates the timeout on the last invocation of urlopen. To ensure the timeout is correctly propagated across all urlopen calls, replace the call_args check with an iteration through mock_urlopen.call_args_list and assert that every call in the list contains the correct timeout value of 20.0. This prevents partial propagation bugs where some calls might have incorrect timeout values while others do not.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modelinfo/cli.py`:
- Around line 110-115: The --timeout argument currently accepts invalid values
like 0, negative numbers, and non-finite floats due to using type=float without
validation. Create a custom validation function that checks if the input is a
positive finite number and raises an appropriate ArgumentTypeError for invalid
values, then replace the type=float parameter in the parser.add_argument call
for --timeout with this custom validation function to ensure invalid timeout
values are rejected at parse time with a clear error message.
---
Nitpick comments:
In `@tests/test_hf_timeout_configurable.py`:
- Around line 7-17: The test_cli_timeout_argument function currently only
verifies that the CLI execution succeeds, but does not actually verify that the
--timeout argument value (20.0) is forwarded to the mocked
fetch_huggingface_repo function. After the main() call completes successfully,
add an assertion on mock_fetch to verify it was called with the timeout
parameter set to 20.0. This ensures the timeout value flows through the entire
call chain from CLI parsing through to the actual fetch function call.
- Around line 44-45: The current assertion using mock_urlopen.call_args only
validates the timeout on the last invocation of urlopen. To ensure the timeout
is correctly propagated across all urlopen calls, replace the call_args check
with an iteration through mock_urlopen.call_args_list and assert that every call
in the list contains the correct timeout value of 20.0. This prevents partial
propagation bugs where some calls might have incorrect timeout values while
others do not.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3440ae91-6099-4962-92d4-5a531bd2e78a
📒 Files selected for processing (4)
README.mdsrc/modelinfo/cli.pysrc/modelinfo/parsers/huggingface.pytests/test_hf_timeout_configurable.py
pipe1os
requested changes
Jun 19, 2026
pipe1os
left a comment
pipe1os
left a comment
Owner
There was a problem hiding this comment.
Thank you for your contribution!
please address the CodeRabbit review points and formatting issues before we merge:
- add custom validation to the
--timeoutargument to reject invalid values like 0, negative numbers, or non-finite floats. - update the unit tests in
test_hf_timeout_configurable.pyto verify:- the
--timeoutargument successfully reachesmock_fetchin the CLI test. - the timeout parameter propagates to all
urlopencalls by checkingmock_urlopen.call_args_list.
- the
- format the codebase by running
uv run ruff format .to pass the linter checks. - resolve the merge conflicts in
huggingface.py.
pipe1os
requested changes
Jun 20, 2026
pipe1os
left a comment
pipe1os
left a comment
Owner
There was a problem hiding this comment.
Thank you for the updates!
just two small things left to fix to pass the ci:
- In
tests/test_parsers.py, please move thefrom modelinfo.parsers.huggingface import _get_hf_endpointimport from line 52 up to the top of the file (around line 4). - please run
uv run ruff format .locally and push the resulting changes to fix the remaining formatting failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--timeoutargument.analyze_modeltofetch_huggingface_repo.Summary by CodeRabbit
New Features
--timeoutCLI argument to configure the network request timeout for Hugging Face Hub operations (defaults to 10.0 seconds).Documentation
--timeoutoption and example values.Tests
--timeoutis accepted by the CLI and correctly applied to Hugging Face fetch calls.