Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix passing top_k parameter for Bedrock Anthropic models #8131

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Fix passing top_k parameter for Bedrock Anthropic models #8131

merged 5 commits into from
Feb 4, 2025

Conversation

vibhavbhat
Copy link
Contributor

@vibhavbhat vibhavbhat commented Jan 31, 2025 •
edited
Loading

Title

Fix passing top_k parameter for Bedrock Anthropic models

Relevant issues

Fixes #7782

Type

🐛 Bug Fix
✅ Test

Changes

The bug arose from the fact that different bedrock models pass in the top_k parameter in different ways.

Specifically the nova model passes in the parameter through

additionalModelRequestFields = {
    "inferenceConfig": {
         "topK": 20
    }
}

and the anthropic model passes it in through

additional_model_fields = {"top_k": top_k}

This PR checks the model types and sets the parameter based on that. Right now, this is a simple if statement, but a long term fix might be to create a new class for each model, and create handling logic for supported / model-specific param within each class. This way the overall converse handler does not need to know about these specifics.

This PR also simplifies the additional_model_fields creation logic

[REQUIRED] Testing - Attach a screenshot of any new tests passing locally

If UI changes, send a screenshot/GIF of working UI fixes

Tested the top_k param for 4 different models w/ real API calls:
image

Tests still passed w/ mocks:
image

@vercel Vercel
Copy link

vercel bot commented Jan 31, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 4, 2025 4:36am

ishaan-jaff
ishaan-jaff requested changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@ishaan-jaff ishaan-jaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, just a couple minor points of feedback

tests/llm_translation/test_bedrock_completion.py Show resolved Hide resolved
litellm/llms/bedrock/chat/converse_transformation.py Show resolved Hide resolved
litellm/llms/bedrock/chat/converse_transformation.py
for k, v in inference_params.items():
if (
k not in supported_converse_params
and k not in supported_tool_call_params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this deletion required ? I'm pretty sure we need to filter our specific guardrail params

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just wanted to simplify the code a bit (using list comprehension instead of adding and then popping from the dict). From my understanding the before and after logic should be functionally equivalent, but if I'm wrong, I can revert it

tests/llm_translation/test_bedrock_completion.py Outdated
"bedrock/mistral.mistral-7b-instruct-v0:2",
]
)
def test_bedrock_top_k(model):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you send a screenshot of this test working for all the API calls here ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think he did

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I have it added it in the description, let me know if you need more screenshots

@krrishdholakia
Copy link
Contributor

Is this okay to merge? @ishaan-jaff

@vibhavbhat vibhavbhat force-pushed the fix_anthropic_top_k_bug branch from c9d0f4d to 5015e87 Compare February 4, 2025 04:35
@vibhavbhat
Copy link
Contributor Author

@ishaan-jaff Just following up to see if this is fine to merge

ishaan-jaff
ishaan-jaff approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@ishaan-jaff ishaan-jaff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, merging into staging branch to run through testing

tests/llm_translation/test_bedrock_completion.py Outdated Show resolved Hide resolved
@ishaan-jaff ishaan-jaff changed the base branch from main to litellm_contributor_feb3 February 4, 2025 06:10
@ishaan-jaff ishaan-jaff merged commit 24260f0 into BerriAI:litellm_contributor_feb3 Feb 4, 2025
2 checks passed
ishaan-jaff added a commit that referenced this pull request Feb 5, 2025
@ishaan-jaff @vibhavbhat
Fix passing top_k parameter for Bedrock Anthropic models (#8131) (#8269)
b965d9b 
* Fix Bedrock Anthropic topK bug

* Remove extra import

* Add unit test + make tests mocked

* Fix camel case

* Fix tests to remove exception handling

Co-authored-by: vibhavbhat <vibhavb00@gmail.com>
@ishaan-jaff ishaan-jaff mentioned this pull request Feb 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krrishdholakia krrishdholakia krrishdholakia left review comments

@spacesphere spacesphere spacesphere left review comments

@ishaan-jaff ishaan-jaff ishaan-jaff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot pass provider-specific parameters to Bedrock Anthropic models
4 participants
@vibhavbhat @krrishdholakia @ishaan-jaff @spacesphere