ValueError in boto3 Bedrock Patch for invoke_model with Cross-Region Inference Model IDs #10772
Assignees
Comments
2 tasks
github-actions bot
pushed a commit
that referenced
this issue
Oct 7, 2024
…ow (#10949) ## What does this PR do? Fixes #10772 Model IDs with cross-region inference would throw because we assumed `modelID` would only have the `provider` and `model_name`, when it could have the region as well. This would result in: ``` File /python3.11/site-packages/ddtrace/contrib/internal/botocore/services/bedrock.py:321, in patched_bedrock_api_call(original_func, instance, args, kwargs, function_vars) 319 params = function_vars.get("params") 320 pin = function_vars.get("pin") --> 321 model_provider, model_name = params.get("modelId").split(".") 322 integration = function_vars.get("integration") 323 submit_to_llmobs = integration.llmobs_enabled and "embed" not in model_name ValueError: too many values to unpack (expected 2) ``` We are not tagging that cross-region inference in this PR, just resolving the error. ### Testing To test this change, a singular `anthropic_message` test case and corresponding cassette was modified. This had an unfortunate side effect of affecting an LLMObs test as well. I can add a different test instead, but I believe that would require an additional 60+ line cassette which isn't totally needed, so I opted for this instead. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) (cherry picked from commit 80a3ee8)
Merged
2 tasks
github-actions bot
pushed a commit
that referenced
this issue
Oct 7, 2024
…ow (#10949) ## What does this PR do? Fixes #10772 Model IDs with cross-region inference would throw because we assumed `modelID` would only have the `provider` and `model_name`, when it could have the region as well. This would result in: ``` File /python3.11/site-packages/ddtrace/contrib/internal/botocore/services/bedrock.py:321, in patched_bedrock_api_call(original_func, instance, args, kwargs, function_vars) 319 params = function_vars.get("params") 320 pin = function_vars.get("pin") --> 321 model_provider, model_name = params.get("modelId").split(".") 322 integration = function_vars.get("integration") 323 submit_to_llmobs = integration.llmobs_enabled and "embed" not in model_name ValueError: too many values to unpack (expected 2) ``` We are not tagging that cross-region inference in this PR, just resolving the error. ### Testing To test this change, a singular `anthropic_message` test case and corresponding cassette was modified. This had an unfortunate side effect of affecting an LLMObs test as well. I can add a different test instead, but I believe that would require an additional 60+ line cassette which isn't totally needed, so I opted for this instead. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) (cherry picked from commit 80a3ee8)
Merged
2 tasks
sabrenner
added a commit
that referenced
this issue
Oct 15, 2024
…ow [backport 2.14] (#10966) Backport 80a3ee8 from #10949 to 2.14. ## What does this PR do? Fixes #10772 Model IDs with cross-region inference would throw because we assumed `modelID` would only have the `provider` and `model_name`, when it could have the region as well. This would result in: ``` File /python3.11/site-packages/ddtrace/contrib/internal/botocore/services/bedrock.py:321, in patched_bedrock_api_call(original_func, instance, args, kwargs, function_vars) 319 params = function_vars.get("params") 320 pin = function_vars.get("pin") --> 321 model_provider, model_name = params.get("modelId").split(".") 322 integration = function_vars.get("integration") 323 submit_to_llmobs = integration.llmobs_enabled and "embed" not in model_name ValueError: too many values to unpack (expected 2) ``` We are not tagging that cross-region inference in this PR, just resolving the error. ### Testing To test this change, a singular `anthropic_message` test case and corresponding cassette was modified. This had an unfortunate side effect of affecting an LLMObs test as well. I can add a different test instead, but I believe that would require an additional 60+ line cassette which isn't totally needed, so I opted for this instead. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
sabrenner
added a commit
that referenced
this issue
Oct 15, 2024
…ow [backport 2.13] (#10965) Backport 80a3ee8 from #10949 to 2.13. ## What does this PR do? Fixes #10772 Model IDs with cross-region inference would throw because we assumed `modelID` would only have the `provider` and `model_name`, when it could have the region as well. This would result in: ``` File /python3.11/site-packages/ddtrace/contrib/internal/botocore/services/bedrock.py:321, in patched_bedrock_api_call(original_func, instance, args, kwargs, function_vars) 319 params = function_vars.get("params") 320 pin = function_vars.get("pin") --> 321 model_provider, model_name = params.get("modelId").split(".") 322 integration = function_vars.get("integration") 323 submit_to_llmobs = integration.llmobs_enabled and "embed" not in model_name ValueError: too many values to unpack (expected 2) ``` We are not tagging that cross-region inference in this PR, just resolving the error. ### Testing To test this change, a singular `anthropic_message` test case and corresponding cassette was modified. This had an unfortunate side effect of affecting an LLMObs test as well. I can add a different test instead, but I believe that would require an additional 60+ line cassette which isn't totally needed, so I opted for this instead. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Sam Brenner <106700075+sabrenner@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary of problem
The boto3 patch logic is broken for bedrock when using
invoke_modelwith cross-region inference. To use an inference profile, a region name is prepended to the modelId e.g.us.anthropic.claude-3-5-sonnet-20240620-v1:0anthropic.claude-3-5-sonnet-20240620-v1:0and this seems to break part of the code that splits the model id into the provider and model name, but this works with boto3.
Which version of dd-trace-py are you using?
2.12.2Which version of pip are you using?
24.2Which libraries and their versions are you using?
How can we reproduce your problem?
Execute the
bedrock-runtimeboto3 clientinvoke_modelwith a model id that uses the cross-region inference model id structure like:us.anthropic.claude-3-5-sonnet-20240620-v1:0.What is the result that you get?
It throws a
ValueErrorexception and breaks our application codeWhat is the result that you expected?
It should not break our application code and should successfully make the request
The text was updated successfully, but these errors were encountered: