Adding more bedrock Claude models with Cross-Region Inference#1650
Closed
brunobpr wants to merge 1 commit intosinaptik-ai:mainfrom
Closed
Adding more bedrock Claude models with Cross-Region Inference#1650brunobpr wants to merge 1 commit intosinaptik-ai:mainfrom
brunobpr wants to merge 1 commit intosinaptik-ai:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 75b3f8a in 1 minute and 24 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. extensions/llms/bedrock/pandasai_bedrock/claude.py:37
- Draft comment:
New model identifiers added. Ensure corresponding unit tests and documentation updates exist for these new models. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment asks the author to "ensure" something, which violates the rule about not asking authors to verify things. Adding model strings to a list is a very simple change that likely doesn't require additional tests - the existing tests would cover the basic functionality. The models are just strings used for validation. The docstring purposely doesn't list specific models since they change frequently.
Perhaps these new models have special handling requirements or different response formats that do need testing? Maybe the documentation should track supported model versions?
Looking at the code, all models are handled exactly the same way - they're just strings used for validation. The code doesn't have any model-specific logic. Documentation listing specific models would get outdated quickly.
The comment should be deleted as it asks for unnecessary work and violates the rule about asking authors to verify/ensure things. The model string additions don't require special testing or documentation updates.
2. extensions/llms/bedrock/pandasai_bedrock/claude.py:37
- Draft comment:
New models added to _supported__models list. Please ensure corresponding unit tests are added to verify cross-region inference functionality. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment is about testing new functionality - regional model variants. However, looking at the code, there's no special handling for regional models - they're just strings in a list. The actual region handling would be done by the bedrock_runtime_client which is passed in. The model strings are only used for validation in init. Adding unit tests just for new model string constants seems unnecessary.
The comment raises a valid point about testing new functionality. Regional inference could have important implications for production use.
While regional testing is important, it would be handled at the AWS SDK/client level, not in this code. This change is just adding new valid model string constants.
The comment should be deleted as it requests tests for functionality that isn't actually implemented in this code - the regional handling is done by AWS's client.
Workflow ID: wflow_v0qO67AOmIQeFWOq
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Contributor
|
@brunobpr PandasAI already supports LiteLLM, which allows you to achieve this functionality with any LLM provider. We recommend using that approach to keep things consistent and flexible. https://docs.pandas-ai.com/v3/large-language-models#how-to-set-up-any-llm%3F |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding the following models:
Claude 3.5 Sonnet v2:
Claude 3.5 Haiku v1 :
Claude 3.7 Sonnet v1:
Important
Add new Claude models for cross-region inference to
_supported__modelsinclaude.py.us.anthropic.claude-3-5-sonnet-20241022-v2:0,apac.anthropic.claude-3-5-sonnet-20241022-v2:0,us.anthropic.claude-3-5-haiku-20241022-v1:0, andus.anthropic.claude-3-7-sonnet-20250219-v1:0to_supported__modelsinclaude.py.This description was created by
for 75b3f8a. It will automatically update as commits are pushed.