Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[bug-fix] Handle BWC for bedrock converse API #3173

Merged
merged 5 commits into from
Oct 29, 2024
Merged

[bug-fix] Handle BWC for bedrock converse API #3173

merged 5 commits into from
Oct 29, 2024

Conversation

pyek-bot
Copy link
Contributor

@pyek-bot pyek-bot commented Oct 25, 2024 •
edited
Loading

Description

Handles serialization and de-serialization of newly added field for bedrock converse API

Related Issues

Resolves #3126

Check List

  • Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@pyek-bot
fix: bwc check for llm messages
5a24892 
Signed-off-by: Pavan Yekbote <[email protected]>
@pyek-bot
Copy link
Contributor Author

Please add 2.x, 2.18 backport labels

@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 25, 2024 21:11 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 25, 2024 21:12 — with GitHub Actions Failure
@pyek-bot
fix: add bwc for llmquestion
c3d5673 
Signed-off-by: Pavan Yekbote <[email protected]>
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 26, 2024 00:23 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 26, 2024 00:23 — with GitHub Actions Failure
b4sjoo
b4sjoo previously approved these changes Oct 26, 2024
View reviewed changes
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 26, 2024 20:24 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 26, 2024 20:24 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 13:19 — with GitHub Actions Failure
@dhrubo-os
Copy link
Collaborator

dhrubo-os commented Oct 28, 2024 •
edited
Loading

can we add corresponding unit test if possible?

@pyek-bot
test: adding ut for bwc
9029ae5 
Signed-off-by: Pavan Yekbote <[email protected]>
@pyek-bot
Copy link
Contributor Author

can we add corresponding unit test if possible?

Added a UT, please review

@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 19:24 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 19:24 — with GitHub Actions Failure
dhrubo-os
dhrubo-os previously approved these changes Oct 28, 2024
View reviewed changes
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 20:28 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 20:28 — with GitHub Actions Failure
b4sjoo
b4sjoo previously requested changes Oct 28, 2024
View reviewed changes
Copy link
Collaborator

@b4sjoo b4sjoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your UT fails, please check and fix

@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 21:54 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 21:54 — with GitHub Actions Failure
@pyek-bot
Copy link
Contributor Author

Your UT fails, please check and fix

Not related to my UT, taking a look at what test is failing

@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 22:24 — with GitHub Actions Failure
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 22:24 — with GitHub Actions Failure
@pyek-bot pyek-bot force-pushed the bwc_fix_bedrock_converse branch from 29dd894 to 4bf57b4 Compare October 28, 2024 22:26
@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 28, 2024 22:26 — with GitHub Actions Failure
@pyek-bot pyek-bot temporarily deployed to ml-commons-cicd-env-require-approval October 28, 2024 22:26 — with GitHub Actions Inactive
@pyek-bot
Copy link
Contributor Author

Tests are fixed. Requesting for re-review @dhrubo-os @b4sjoo

@pyek-bot pyek-bot had a problem deploying to ml-commons-cicd-env-require-approval October 29, 2024 15:57 — with GitHub Actions Failure
@dhrubo-os dhrubo-os dismissed b4sjoo’s stale review October 29, 2024 16:01

Tests are passing now.

@dhrubo-os dhrubo-os merged commit 24d2640 into opensearch-project:main Oct 29, 2024
6 of 7 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 29, 2024
@pyek-bot @github-actions
[bug-fix] Handle BWC for bedrock converse API (#3173)
bf77d1b 
* fix: bwc check for llm messages

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: add bwc for llmquestion

Signed-off-by: Pavan Yekbote <[email protected]>

* test: adding ut for bwc

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: test case which counts optional occurence

Signed-off-by: Pavan Yekbote <[email protected]>

---------

Signed-off-by: Pavan Yekbote <[email protected]>
(cherry picked from commit 24d2640)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 29, 2024
Merged
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 29, 2024
@pyek-bot @github-actions
[bug-fix] Handle BWC for bedrock converse API (#3173)
1a639eb 
* fix: bwc check for llm messages

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: add bwc for llmquestion

Signed-off-by: Pavan Yekbote <[email protected]>

* test: adding ut for bwc

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: test case which counts optional occurence

Signed-off-by: Pavan Yekbote <[email protected]>

---------

Signed-off-by: Pavan Yekbote <[email protected]>
(cherry picked from commit 24d2640)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 29, 2024
Merged
dhrubo-os pushed a commit that referenced this pull request Oct 29, 2024
@opensearch-trigger-bot @pyek-bot
[bug-fix] Handle BWC for bedrock converse API (#3173) (#3183)
6c9415a 
* fix: bwc check for llm messages

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: add bwc for llmquestion

Signed-off-by: Pavan Yekbote <[email protected]>

* test: adding ut for bwc

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: test case which counts optional occurence

Signed-off-by: Pavan Yekbote <[email protected]>

---------

Signed-off-by: Pavan Yekbote <[email protected]>
(cherry picked from commit 24d2640)

Co-authored-by: Pavan Yekbote <[email protected]>
dhrubo-os pushed a commit that referenced this pull request Oct 29, 2024
@opensearch-trigger-bot @pyek-bot
[bug-fix] Handle BWC for bedrock converse API (#3173) (#3184)
8414e70 
* fix: bwc check for llm messages

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: add bwc for llmquestion

Signed-off-by: Pavan Yekbote <[email protected]>

* test: adding ut for bwc

Signed-off-by: Pavan Yekbote <[email protected]>

* fix: test case which counts optional occurence

Signed-off-by: Pavan Yekbote <[email protected]>

---------

Signed-off-by: Pavan Yekbote <[email protected]>
(cherry picked from commit 24d2640)

Co-authored-by: Pavan Yekbote <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhrubo-os dhrubo-os dhrubo-os approved these changes

@ylwu-amzn ylwu-amzn ylwu-amzn approved these changes

@b4sjoo b4sjoo b4sjoo left review comments

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

@sam-herman sam-herman Awaiting requested review from sam-herman

@xinyual xinyual Awaiting requested review from xinyual xinyual is a code owner

Assignees
No one assigned
Labels
backport 2.x backport 2.18
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] RAG: Handle BWC for bedrock converse API
4 participants
@pyek-bot @dhrubo-os @ylwu-amzn @b4sjoo