-
Notifications
You must be signed in to change notification settings - Fork 693
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.
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
feat: added support for A21 and Amazon Titan models via bedrock api #1101
Conversation
Signed-off-by: Yomesh Shah <[email protected]>
Signed-off-by: Yomesh Shah <[email protected]>
Signed-off-by: Yomesh Shah <[email protected]>
@AlexsJones did you get a chance to review this for the changes and if it is ok to merge now ? |
I need to test it first, will try to get around to it ASAP |
@AlexsJones just adding some screenshots of the runs if that helps. |
If you're looking for a workaround, would suggest using LiteLLM to proxy Bedrock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good start! i left some comments
Can you respond to comments and we will merge this next week @awsyshah ? |
…in the code Signed-off-by: Yomesh Shah <[email protected]>
made the key suggest improvement of moving some of the constants to config. The other vars are as per the API or not need outside of the case and hence declared within it. Please review. |
comments are not fixed |
Hi @JuHyung-Son , Sorry for the trouble there, can I request which are the specific ones you want changed. The first action of moving the constants like maxTokens to config is done right ? or is my understanding / implementation incorrect ? For Comment 2 , the stop_reason , that call used is as per the documentation from :https://docs.aws.amazon.com/bedrock/latest/userguide/model-parameters-anthropic-claude-messages.html For Comment 3: The type you have commented on is only used once and that is if it is a a21 model so its defined within the area so as its only ever used there. The rest of the code has not use of it , is it still better to just define it up top even is only ever to be used if the model is one of the A21 one and may not be used otherwise ? Let me know and I will make the changes so we can have this merged. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i got it.
LGTM!
…1101) * feat: added support for A21 and Amazon Titan models via bedrock api Signed-off-by: Yomesh Shah <[email protected]> * fix: response type for diffrent models and use of constant for top_P Signed-off-by: Yomesh Shah <[email protected]> * fix: constant for top_P as int vs string Signed-off-by: Yomesh Shah <[email protected]> * feat: moved topP and maxTokens to config rather than being constants in the code Signed-off-by: Yomesh Shah <[email protected]> --------- Signed-off-by: Yomesh Shah <[email protected]> Co-authored-by: Alex Jones <[email protected]> Signed-off-by: AlexsJones <[email protected]>
Closes #1076
📑 Description
Included support for A21 & Amazon Titan Model through the bedrock api, using a case structure to enable future flexibility to continue adding other models available. As Bedrock supports quite a few models not adding them all in a single merge.
✅ Checks
ℹ Additional Information