Repository navigation
chore: add ruff + mypy Python linting with pre-commit and CI - #340
Conversation
Add pre-commit hooks (ruff check + ruff format) and a CI python-lint job with ruff and mypy. Bump minimum Python to 3.12.
Modernize typing (Optional[X] -> X | None, List -> list, Dict -> dict), fix import sorting, remove unused imports/variables, fix line lengths, and apply consistent formatting to all Python files.
github-actions
Bot
added
python-bindings
📝 WalkthroughWalkthroughThis PR modernizes the Python codebase to version 3.12+, adding linting infrastructure (mypy, ruff) via CI/CD and configuration files, updating all type annotations to PEP 604 union syntax and built-in generics (list, dict), and refactoring key modules for improved type safety and error handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @CatherineSue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the quality and consistency of the Python codebase by integrating modern linting, formatting, and static type-checking tools. By adopting Ruff and Mypy, and updating the Python version to 3.12, the project benefits from automated code quality enforcement, improved readability, and early detection of potential issues, streamlining development and maintenance workflows. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bindings/python/tests/test_startup_sequence.py (1)
839-854:⚠️ Potential issue | 🟠 MajorNested function will not be executed by pytest.
test_validation_error_handlingis defined insidetest_launch_server_process_and_cleanupat line 839, making it a nested function rather than a test method. It also usesselfwithout being part of a class. This test will never be discovered or executed by pytest.This appears to be an indentation error—the function should likely be a method of a test class (e.g.,
TestStartupErrorHandling) or moved to module level as a standalone test function.Proposed fix: Move to class or module level
If it belongs in
TestStartupErrorHandling, move it there:class TestStartupErrorHandling: """Test startup error handling logic.""" + def test_validation_error_handling(self): + """Test error handling when validation fails.""" + args = RouterArgs( + pd_disaggregation=True, + prefill_urls=[], + decode_urls=[], + service_discovery=False, + ) + + with patch("smg.launch_router.logger") as mock_logger: + with pytest.raises(ValueError, match="PD disaggregation mode requires --prefill"): + launch_router(args) + + # Should log error for validation failures + mock_logger.error.assert_called_once() + def test_router_creation_error_handling(self):And remove it from inside
test_launch_server_process_and_cleanup.bindings/python/src/smg/launch_router.py (1)
18-46:⚠️ Potential issue | 🟡 MinorReturn type and docstring are inconsistent with actual behavior.
The function signature declares
-> Router | Noneand the docstring states "Returns: Router instance if successful, None if failed". However, the function:
- Returns
Noneimplicitly on success (no return statement afterrouter.start())- Raises an exception on failure (re-raises after logging)
The function never returns a
Routerinstance. Consider either:
- Returning the router instance after starting it, or
- Updating the return type to
-> Noneand fixing the docstringOption 1: Return the router instance
router = Router.from_args(router_args) router.start() + return router except Exception as e: logger.error(f"Error starting router: {e}") raise eOption 2: Fix return type and docstring
-def launch_router(args: argparse.Namespace) -> Router | None: +def launch_router(args: argparse.Namespace) -> None: """ Launch the SMG router with the configuration from parsed arguments. Args: args: Namespace object containing router configuration Can be either raw argparse.Namespace or converted RouterArgs - Returns: - Router instance if successful, None if failed + Raises: + RuntimeError: If Rust Router is not installed + Exception: If router initialization or startup fails """bindings/python/src/smg/router.py (1)
17-31:⚠️ Potential issue | 🟡 MinorReturn type annotation is incorrect.
The function returns
Nonewhenpolicy_str is None(lines 19-20), but the return type is declared as-> PolicyTypewithout allowingNone. This is a type error that mypy should catch.Proposed fix
-def policy_from_str(policy_str: str | None) -> PolicyType: +def policy_from_str(policy_str: str | None) -> PolicyType | None:
🤖 Fix all issues with AI agents
In `@bindings/python/src/smg/router_args.py`:
- Around line 619-638: The CLI help string for the disable-circuit-breaker arg
in RouterArgs currently uses Rust-specific wording ("u32::MAX"); update the help
text in the add_argument call for f"--{prefix}disable-circuit-breaker" to a
language-agnostic phrasing such as "Disable circuit breaker (equivalent to
setting cb_failure_threshold to the maximum 32-bit unsigned value)" or "Disable
circuit breaker by setting cb_failure_threshold to a very large value", so users
of the Python CLI aren’t exposed to Rust-specific notation; locate the change
near the RouterArgs code where health_group and the disable-circuit-breaker
argument are defined.
In `@e2e_test/fixtures/hooks.py`:
- Line 27: The type annotation for _worker_counts is incorrect: it currently
uses tuple[ConnectionMode, WorkerType] but the code uses 3-tuples including a
model_id string; update the annotation to dict[tuple[ConnectionMode, WorkerType,
str], int] (referencing the _worker_counts variable and the ConnectionMode and
WorkerType types and the model_id key used across the file) so the declared type
matches actual usage.
🧹 Nitpick comments (3)
e2e_test/bindings_go/conftest.py (1)
142-163: Consider adding type parameter for consistency.Line 142's return type is properly parameterized, but line 163 uses an unparameterized
list. For consistency with the PR's type modernization goal and better static analysis:- instances: list = [] + instances: list[ModelInstance] = []mypy.ini (1)
1-6: Permissive mypy configuration suitable for initial adoption.The configuration is intentionally lenient (
check_untyped_defs = False,warn_return_any = False) which is reasonable for introducing mypy to an existing codebase. Consider adding a comment noting the intent to gradually tighten these settings as type coverage improves.📝 Suggested documentation comment
[mypy] +# NOTE: Permissive configuration for initial adoption. +# TODO: Tighten settings as type coverage improves: +# - check_untyped_defs = True +# - warn_return_any = True python_version = 3.12 ignore_missing_imports = True warn_return_any = False warn_unused_configs = True check_untyped_defs = False.github/workflows/pr-test-rust.yml (1)
57-57: Consider pinning ruff and mypy versions for CI stability.Installing unpinned
ruffandmypymeans CI behavior may change unexpectedly when new versions are released (e.g., new lint rules, stricter checks). Consider pinning to specific versions that match what's inpyproject.tomldev dependencies:🔧 Suggested fix
- name: Install linting tools - run: pip install ruff mypy + run: pip install ruff==0.8.6 mypy==1.14.1Alternatively, install from the project's dev dependencies to ensure consistency with local development.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to improve Python code quality by introducing ruff for linting/formatting and mypy for type checking. The update of the minimum Python version to 3.12 and the application of automated fixes across the codebase are valuable changes. The configurations for the new tools are sensible starting points. I have one suggestion regarding the pre-commit configuration to ensure it works as expected for all developers.
- Migrate str+Enum to StrEnum (ruff UP042) - Fix _worker_counts type annotation to match 3-tuple usage - Replace Rust-specific u32::MAX in CLI help with generic wording
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/python/tests/test_startup_sequence.py (1)
839-854:⚠️ Potential issue | 🟡 MinorMove
test_validation_error_handlingto theTestStartupErrorHandlingclass.This test is currently nested inside the top-level function
test_launch_server_process_and_cleanup(line 777), making it undiscoverable by pytest. As a nested function takingselfas a parameter, it cannot execute. Move it to theTestStartupErrorHandlingclass (which exists at line 585) where it belongs as a proper test method.🐛 Proposed fix: Move to TestStartupErrorHandling class
Move this function to be a method of the
TestStartupErrorHandlingclass (after line 605):class TestStartupErrorHandling: """Test startup error handling logic.""" def test_router_creation_error_handling(self): ... def test_router_start_error_handling(self): ... + def test_validation_error_handling(self): + """Test error handling when validation fails.""" + args = RouterArgs( + pd_disaggregation=True, + prefill_urls=[], + decode_urls=[], + service_discovery=False, + ) + + with patch("smg.launch_router.logger") as mock_logger: + with pytest.raises(ValueError, match="PD disaggregation mode requires --prefill"): + launch_router(args) + + # Should log error for validation failures + mock_logger.error.assert_called_once() +Remove it from inside
test_launch_server_process_and_cleanup.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bindings/python/src/smg/launch_router.py (1)
18-28:⚠️ Potential issue | 🟡 MinorUpdate the docstring to match the no‑return behavior.
launch_routernow returnsNone, but the docstring still describes a Router return value.Suggested fix
- Returns: - Router instance if successful, None if failed + Returns: + None. Raises on failure.e2e_test/infra/model_pool.py (1)
76-84:⚠️ Potential issue | 🟠 MajorClarify and unify PD worker key construction in ModelPool.
WorkerIdentity.keyfor non-REGULAR workers includes the index (e.g.,llama-8b:http:prefill_0), butModelPool._get_unlockedand_launch_modelconstruct lookup keys without the index (llama-8b:http:prefill). While the codebase avoids callingget()with PREFILL/DECODE types (usingget_workers_by_type()instead), this inconsistency is a design hazard: if future code callsget(model_id, mode, WorkerType.PREFILL), it will silently return only the first matching worker, hiding the existence of others. Either include the index in all PD worker keys or explicitly document thatget()is not intended for PREFILL/DECODE workers.
🤖 Fix all issues with AI agents
In `@bindings/python/src/smg/launch_server.py`:
- Around line 183-186: The signal handlers currently call
cleanup_processes(server_processes) but do not terminate the main process;
change the handlers registered via signal.signal for SIGINT, SIGTERM and SIGQUIT
to invoke cleanup_processes(server_processes) and then exit the process (e.g.,
call sys.exit with an appropriate exit code) so the main process does not
continue running without workers; update the lambdas or replace them with a
small helper function (referencing cleanup_processes and the signal registration
sites) to ensure cleanup is performed followed by an immediate exit.
In `@e2e_test/infra/gateway.py`:
- Around line 272-276: The current regular-mode validation only checks for None
and allows an empty list; update the guard around worker_urls in the same block
that raises for model_path to ensure worker_urls is a non-empty sequence (e.g.,
if not worker_urls or len(worker_urls) == 0: raise ValueError("worker_urls must
be a non-empty list for regular mode")). Locate the check that references
worker_urls and model_path in gateway.py and replace or augment the existing "if
worker_urls is None" check with a truthiness/length check (optionally also
validate element types/strings) so an empty [] is rejected.
🧹 Nitpick comments (1)
e2e_test/infra/run_eval.py (1)
127-136: Consider logging a warning when score is None.Silently defaulting
result.scorefromNoneto0.0masks the distinction between "evaluation produced a zero score" and "evaluation failed to compute a score." In CI/CD environments, this could hide evaluation failures.Consider adding a warning when the fallback occurs:
# Build metrics metrics = result.metrics.copy() if result.metrics else {} + if result.score is None: + logger.warning("%s eval returned no score, defaulting to 0.0", eval_name) metrics["score"] = result.score if result.score is not None else 0.0
- Update launch_router docstring to match None return type - Fix signal handlers to exit after cleanup in launch_server - Guard against empty worker_urls in gateway - Log warning when eval score is None - Move test_validation_error_handling to TestStartupErrorHandling class
The test was previously dead code (nested function inside another test, undiscoverable by pytest). Moving it to TestStartupErrorHandling made it run for the first time, but it only patched logger — not Router. If validation didn't raise, Router.from_args().start() would block forever.
This test was originally dead code (nested function, never run by pytest). _validate_router_args() only checks policy warnings — it does not validate that prefill/decode URLs are non-empty, so the expected ValueError was never raised.
5ba08ad to
79c153f
Compare
Cover the pid-is-None guard, SIGTERM ProcessLookupError, and SIGKILL ProcessLookupError paths to bring coverage back above 80%.
79c153f to
37b0785
Compare
Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
2 participants
Description
Problem
The Python code in
e2e_test/,bindings/python/, andscripts/had no automated linting or formatting enforcement, leading to inconsistent style, deprecated typing patterns, and unused imports accumulating over time.Solution
Add ruff (lint + format) as a pre-commit hook and CI check, plus mypy for type checking in CI. Bump minimum Python version from 3.9 (EOL) to 3.12.
Changes
ruff.toml— New config: Python 3.12 target, line-length 100 (code) / 120 (comments/docstrings), rules: E, F, I, W, UPmypy.ini— New permissive mypy config for CI.pre-commit-config.yaml— Removed global exclude for Python dirs, addedruffandruff-formathooks.github/workflows/pr-test-rust.yml— Addedpython-lintCI job (ruff check, ruff format --check, mypy)e2e_test/pyproject.toml/bindings/python/pyproject.toml— Bumpedrequires-pythonto>=3.12, added mypy + ruff dev depsOptional[X]→X | None,List→list,Dict→dict,Tuple→tupleTest Plan
ruff check e2e_test/ bindings/python/ scripts/→ All checks passedruff format --check e2e_test/ bindings/python/ scripts/→ 68 files already formattedpython-lintjob validates on every PR going forwardChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesruff checkpassesruff format --checkpassesSummary by CodeRabbit
Release Notes
Chores
Refactor
|operator instead ofUnion).Bug Fixes