Fix health check stripping bedrock/ prefix from model name#24716
Fix health check stripping bedrock/ prefix from model name#24716joaquinhuigomez wants to merge 3 commits intoBerriAI:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a one-line bug in Key changes:
Confidence Score: 5/5This PR is safe to merge — it is a minimal, targeted bug fix with comprehensive test coverage. The change is a single-line fix that corrects a clear regression (stripped provider prefix). All existing test assertions have been properly updated to reflect the corrected behavior, not weakened to mask a regression. No new network calls, no architectural changes, no backwards-incompatible behavior for non-Bedrock providers. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/health_check.py | One-line fix: re-adds bedrock/ prefix after region-stripping, preventing get_llm_provider() failures for models not in the cost map. |
| tests/litellm_utils_tests/test_health_check.py | All Bedrock-related assertions updated from the old (stripped) behavior to the corrected (prefix-preserved) behavior; also includes minor Black-style reformatting. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["litellm_params['model']"] --> B{starts with bedrock/?}
B -- No --> G[Return params unchanged]
B -- Yes --> C[Strip 'bedrock/' prefix]
C --> D[Split remaining path by '/']
D --> E["Filter out AWS region segments\n(e.g. us-west-2, eu-central-1)"]
E --> F["Re-join parts and prepend 'bedrock/'"]
F --> H["litellm_params['model'] = 'bedrock/' + filtered_model"]
H --> I[Return params]
G --> I
Reviews (3): Last reviewed commit: "Fix black formatting in test_health_chec..." | Re-trigger Greptile
|
|
||
| model = "/".join(filtered_parts) | ||
| litellm_params["model"] = model | ||
| litellm_params["model"] = "bedrock/" + model |
There was a problem hiding this comment.
Existing tests expect old (no-prefix) behavior and would fail
The fix correctly re-adds bedrock/ prefix, but all existing Bedrock-related assertions in tests/litellm_utils_tests/test_health_check.py encode the old (pre-fix) behavior and would now fail:
- Line 348:
assert updated_params["model"] == "anthropic.claude-3-7-sonnet-20250219-v1:0"— would now get"bedrock/anthropic.claude-3-7-sonnet-20250219-v1:0" - Line 357:
assert updated_params["model"] == "us.anthropic.claude-3-5-sonnet-20240620-v1:0"— same issue - Line 365:
assert updated_params["model"] == "anthropic.claude-3-5-sonnet-20240620-v1:0"— same issue - Lines 395, 403, 412, 424, 432, 442, 454, 463, 473 — all assert model names without the
bedrock/prefix
Every assertion in the "Bedrock" section of test_update_litellm_params_for_health_check needs to be updated to reflect the new expected behavior, e.g.:
# Before (old broken behavior)
assert updated_params["model"] == "anthropic.claude-3-7-sonnet-20250219-v1:0"
# After (correct behavior, matches the fix)
assert updated_params["model"] == "bedrock/anthropic.claude-3-7-sonnet-20250219-v1:0"Similarly, sub-prefix variants like "us.anthropic...", route variants like "converse/...", and handler variants like "llama/arn:..." all need the bedrock/ prepended in their assertions to match the fixed output.
Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)
|
Closing — lint noise is making this hard to review. Happy to revisit. |
Fixes #24694
_update_litellm_params_for_health_checkstrips thebedrock/prefix from the model name to extract the base model, but never re-adds it when assigning back tolitellm_params["model"]. This causesget_llm_provider()to fail for models not in the cost map since there's no provider prefix to split on.Re-added the
bedrock/prefix after processing so the model name retains its provider routing information.