mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-05 08:07:05 +00:00
fix(complexity_router): Address Greptile review feedback
Fixes 5 issues flagged in code review: 1. **Mutable singleton mutation bug** - Now always creates a new ComplexityRouterConfig instance instead of reusing DEFAULT_COMPLEXITY_CONFIG singleton, preventing cross-instance config pollution. 2. **Substring matching false positives** - Added word boundaries (spaces) to short keywords like 'ok', 'try', 'api', 'git', 'node', 'java', 'vue' to prevent matching within longer words (e.g., 'capital' matching 'api'). 3. **Redundant message extraction** - Simplified to single reverse loop that extracts both last user message and last system prompt efficiently. 4. **Unused imports** - Removed unused DEFAULT_CREATIVE_KEYWORDS and DEFAULT_MULTI_STEP_PATTERNS imports. 5. **Missing async_pre_routing_hook tests** - Added comprehensive tests for: - Multi-turn conversations - List-type content handling - No user message case - Empty string content - Message preservation - Singleton mutation prevention
This commit is contained in:
parent
9d575bc2c9
commit
35aa12200a
3 changed files with 143 additions and 25 deletions
|
|
@ -18,9 +18,6 @@ from .config import (
|
|||
ComplexityRouterConfig,
|
||||
ComplexityTier,
|
||||
DEFAULT_CODE_KEYWORDS,
|
||||
DEFAULT_COMPLEXITY_CONFIG,
|
||||
DEFAULT_CREATIVE_KEYWORDS,
|
||||
DEFAULT_MULTI_STEP_PATTERNS,
|
||||
DEFAULT_REASONING_KEYWORDS,
|
||||
DEFAULT_SIMPLE_KEYWORDS,
|
||||
DEFAULT_TECHNICAL_KEYWORDS,
|
||||
|
|
@ -79,11 +76,11 @@ class ComplexityRouter(CustomLogger):
|
|||
self.model_name = model_name
|
||||
self.litellm_router_instance = litellm_router_instance
|
||||
|
||||
# Parse config
|
||||
# Parse config - always create a new instance to avoid singleton mutation
|
||||
if complexity_router_config:
|
||||
self.config = ComplexityRouterConfig(**complexity_router_config)
|
||||
else:
|
||||
self.config = DEFAULT_COMPLEXITY_CONFIG
|
||||
self.config = ComplexityRouterConfig()
|
||||
|
||||
# Override default_model if provided
|
||||
if default_model:
|
||||
|
|
@ -342,27 +339,19 @@ class ComplexityRouter(CustomLogger):
|
|||
)
|
||||
return None
|
||||
|
||||
# Extract user message and optional system prompt
|
||||
# Extract the last user message and the last system prompt
|
||||
user_message = ""
|
||||
system_prompt = None
|
||||
|
||||
for msg in messages:
|
||||
for msg in reversed(messages):
|
||||
role = msg.get("role", "")
|
||||
content = msg.get("content", "")
|
||||
if isinstance(content, str):
|
||||
if role == "user":
|
||||
if role == "user" and not user_message:
|
||||
user_message = content
|
||||
elif role == "system":
|
||||
elif role == "system" and system_prompt is None:
|
||||
system_prompt = content
|
||||
|
||||
# Use the last user message if there are multiple
|
||||
for msg in reversed(messages):
|
||||
if msg.get("role") == "user":
|
||||
content = msg.get("content", "")
|
||||
if isinstance(content, str):
|
||||
user_message = content
|
||||
break
|
||||
|
||||
if not user_message:
|
||||
verbose_router_logger.debug(
|
||||
"ComplexityRouter: No user message found, skipping routing"
|
||||
|
|
|
|||
|
|
@ -23,13 +23,13 @@ class ComplexityTier(str, Enum):
|
|||
DEFAULT_CODE_KEYWORDS: List[str] = [
|
||||
"function", "class", "def ", "const ", "let ", "var ",
|
||||
"import ", "export ", "return ", "async ", "await ",
|
||||
"try", "catch", "exception", "error", "debug",
|
||||
"api", "endpoint", "request", "response",
|
||||
"database", "sql", "query", "schema",
|
||||
" try ", "catch", "exception", " error ", "debug",
|
||||
" api ", "endpoint", "request", "response",
|
||||
"database", " sql ", "query ", "schema",
|
||||
"algorithm", "implement", "refactor", "optimize",
|
||||
"python", "javascript", "typescript", "java", "rust", "golang",
|
||||
"react", "vue", "angular", "node", "docker", "kubernetes",
|
||||
"git", "commit", "merge", "branch", "pull request",
|
||||
"python", "javascript", "typescript", " java ", "rust", "golang",
|
||||
"react", " vue ", "angular", " node ", "docker", "kubernetes",
|
||||
" git ", "commit", "merge", "branch", "pull request",
|
||||
]
|
||||
|
||||
DEFAULT_REASONING_KEYWORDS: List[str] = [
|
||||
|
|
@ -60,7 +60,7 @@ DEFAULT_SIMPLE_KEYWORDS: List[str] = [
|
|||
"yes or no", "true or false",
|
||||
"simple", "brief", "short", "quick",
|
||||
"hello", "hi ", "hey ", "thanks", "thank you",
|
||||
"goodbye", "bye", "ok", "okay",
|
||||
"goodbye", "bye ", " ok ", "okay",
|
||||
]
|
||||
|
||||
DEFAULT_MULTI_STEP_PATTERNS: List[str] = [
|
||||
|
|
|
|||
|
|
@ -99,7 +99,9 @@ class TestComplexityRouterInit:
|
|||
litellm_router_instance=mock_router_instance,
|
||||
)
|
||||
assert router.model_name == "test-router"
|
||||
assert router.config == DEFAULT_COMPLEXITY_CONFIG
|
||||
# Should have equivalent default values but NOT be the same instance
|
||||
assert router.config.tiers == DEFAULT_COMPLEXITY_CONFIG.tiers
|
||||
assert router.config is not DEFAULT_COMPLEXITY_CONFIG # Not a singleton
|
||||
|
||||
def test_init_with_default_model(self, mock_router_instance, basic_config):
|
||||
"""Test initialization with default_model override."""
|
||||
|
|
@ -441,6 +443,133 @@ class TestConfigOverrides:
|
|||
assert any("long" in s.lower() if s else False for s in signals), f"Expected 'long' signal, got {signals}"
|
||||
|
||||
|
||||
class TestAsyncPreRoutingHookEdgeCases:
|
||||
"""Test edge cases for async_pre_routing_hook method."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pre_routing_hook_multi_turn_conversation(self, complexity_router):
|
||||
"""Test pre-routing hook with multi-turn conversation uses last user message."""
|
||||
messages = [
|
||||
{"role": "user", "content": "What is Python?"},
|
||||
{"role": "assistant", "content": "Python is a programming language."},
|
||||
{"role": "user", "content": "Hello!"}, # Last user message - simple
|
||||
]
|
||||
result = await complexity_router.async_pre_routing_hook(
|
||||
model="test-model",
|
||||
request_kwargs={},
|
||||
messages=messages,
|
||||
)
|
||||
assert result is not None
|
||||
assert result.model == "gpt-4o-mini" # SIMPLE tier based on last message
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pre_routing_hook_list_content_skipped(self, complexity_router):
|
||||
"""Test pre-routing hook handles list content (skips non-string)."""
|
||||
messages = [
|
||||
{"role": "user", "content": [{"type": "text", "text": "What is this?"}]},
|
||||
{"role": "user", "content": "Hello!"},
|
||||
]
|
||||
result = await complexity_router.async_pre_routing_hook(
|
||||
model="test-model",
|
||||
request_kwargs={},
|
||||
messages=messages,
|
||||
)
|
||||
assert result is not None
|
||||
# Should use the string content "Hello!"
|
||||
assert result.model == "gpt-4o-mini"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pre_routing_hook_no_user_message(self, complexity_router):
|
||||
"""Test pre-routing hook returns None when no user message found."""
|
||||
messages = [
|
||||
{"role": "system", "content": "You are helpful."},
|
||||
{"role": "assistant", "content": "Hello!"},
|
||||
]
|
||||
result = await complexity_router.async_pre_routing_hook(
|
||||
model="test-model",
|
||||
request_kwargs={},
|
||||
messages=messages,
|
||||
)
|
||||
assert result is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pre_routing_hook_only_list_content(self, complexity_router):
|
||||
"""Test pre-routing hook returns None when all user content is list type."""
|
||||
messages = [
|
||||
{"role": "user", "content": [{"type": "text", "text": "Hello"}]},
|
||||
]
|
||||
result = await complexity_router.async_pre_routing_hook(
|
||||
model="test-model",
|
||||
request_kwargs={},
|
||||
messages=messages,
|
||||
)
|
||||
# Should return None since we can't extract string content
|
||||
assert result is None
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pre_routing_hook_preserves_messages(self, complexity_router):
|
||||
"""Test pre-routing hook preserves original messages in response."""
|
||||
messages = [
|
||||
{"role": "system", "content": "Be helpful"},
|
||||
{"role": "user", "content": "Hello!"},
|
||||
]
|
||||
result = await complexity_router.async_pre_routing_hook(
|
||||
model="test-model",
|
||||
request_kwargs={},
|
||||
messages=messages,
|
||||
)
|
||||
assert result is not None
|
||||
assert result.messages == messages
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pre_routing_hook_empty_string_content(self, complexity_router):
|
||||
"""Test pre-routing hook handles empty string content."""
|
||||
messages = [
|
||||
{"role": "user", "content": ""},
|
||||
]
|
||||
result = await complexity_router.async_pre_routing_hook(
|
||||
model="test-model",
|
||||
request_kwargs={},
|
||||
messages=messages,
|
||||
)
|
||||
# Empty content should still route (to SIMPLE tier)
|
||||
assert result is not None
|
||||
|
||||
|
||||
class TestSingletonMutation:
|
||||
"""Test that the config singleton is not mutated."""
|
||||
|
||||
def test_default_config_not_mutated(self, mock_router_instance):
|
||||
"""Test that creating routers without config doesn't mutate defaults."""
|
||||
from litellm.router_strategy.complexity_router.config import ComplexityRouterConfig
|
||||
|
||||
# Get original default
|
||||
original_default = ComplexityRouterConfig().default_model
|
||||
|
||||
# Create router with empty config and custom default_model
|
||||
router1 = ComplexityRouter(
|
||||
model_name="test-router-1",
|
||||
litellm_router_instance=mock_router_instance,
|
||||
complexity_router_config=None,
|
||||
default_model="custom-fallback",
|
||||
)
|
||||
|
||||
# Create another router without config
|
||||
router2 = ComplexityRouter(
|
||||
model_name="test-router-2",
|
||||
litellm_router_instance=mock_router_instance,
|
||||
complexity_router_config=None,
|
||||
)
|
||||
|
||||
# Router2 should have fresh defaults, not router1's custom default_model
|
||||
# Create a fresh config to check
|
||||
fresh_config = ComplexityRouterConfig()
|
||||
assert fresh_config.default_model == original_default
|
||||
assert router1.config.default_model == "custom-fallback"
|
||||
# Router2's config should be independent
|
||||
assert router2.config is not router1.config
|
||||
|
||||
|
||||
class TestEdgeCases:
|
||||
"""Test edge cases and error handling."""
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue