feat: Added support for Bedrock invoke model user request #2721
Conversation
|
|
| @@ -77,15 +77,19 @@ class BedrockCommand { | |||
| } else if (this.isCohereEmbed() === true) { | |||
| result = this.#body.texts.join(' ') | |||
| } else if ( | |||
| this.isClaude() === true || | |||
| (this.isClaude() === true && this.isClaude3() === false) || | |||
There was a problem hiding this comment.
Why is this condition change necessary? The isClaude method does not match for v3, or it shouldn't.
There was a problem hiding this comment.
Thank you for your review, and yes, you are correct! this.isClaude3() === false is unneeded at this time, and is pushing the solution into redundancy. Im happy to remove this change from the PR.
For context:
I want to add this change because I think it improves readability by explicitly calling out this will not match claude 3.x.
Currently, Claude is the only llm that we treat as a separate llm based on its version. I get that is because of the API. But thinking about the future would isClaude support anthropic.claude-v4 or would we need to continue the pattern of creating a new possible llm isClaude4?
Furthermore, I think this will help developers of all exp. levels to quickly consume the code and be able to contribute.
There was a problem hiding this comment.
I think it depends on how they provide these things via the AWS API. We initially had one "Claude". So we implemented an isClaude method. Then they put out a "Claude v3" with a completely different identifier. So we added an isClaude3 method.
I think this discussion is valid, but probably one to have separate from this PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2721 +/- ##
==========================================
- Coverage 97.22% 97.20% -0.02%
==========================================
Files 290 294 +4
Lines 45882 46198 +316
==========================================
+ Hits 44608 44908 +300
- Misses 1274 1290 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@jsumners-nr. Thank you for the review, I have included tests in my latest commit. the tests are to cover the changes in Claude 3.5 and the invoke model. In addition I have updated this PR's description calling out the scope of this change. |
jsumners-nr
changed the title
cmcadams-newrelic
force-pushed
the
NR-326020-support-user-input-bedrock
branch
from
e5975b9 to
66dfcb3
Compare
Description
This Pr will improve the support for using the invoke model with bedrock LLM. the Invoke API expects the body to be of type
{[content: "user or assistant message"]}.We will now check for the type of the input if the type is a string we will return the value as is. If the param is an object we will attempt to reduce the object to just the message value.
How to Test
run test suite.
Related Issues
https://new-relic.atlassian.net/browse/NR-326020