-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
fix: better bedrock message handler close #10976 #11317
fix: better bedrock message handler close #10976 #11317
Conversation
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
@ybalbert001 Please take a look at this. |
Signed-off-by: yihong0618 <[email protected]>
I think it's better to merge two user prompts into one. a 'placeholder' text may change the model's behavior. btw, its limit is from Claude, not the bedrock. |
Signed-off-by: yihong0618 <[email protected]>
fixed and turn out most of bedrock model had this problem |
Signed-off-by: yihong0618 <[email protected]>
OK |
I test it in my environment. it's ok, I suggest wrap this code in function with better naming. |
I think code leave here with comments is better since it maybe a temp solution and when upstream update we can delete them |
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.
LGTM
Summary
add a placeholder to bypass Bedrock models not support two roles next to each other
close #10976
Tip
Close issue syntax:
Fixes #<issue number>
orResolves #<issue number>
, see documentation for more details.Screenshots
Checklist
Important
Please review the checklist below before submitting your pull request.
dev/reformat
(backend) andcd web && npx lint-staged
(frontend) to appease the lint gods