-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security solution] ChatBedrockConverse
#200042
Conversation
...ackages/kbn-langchain/server/language_models/chat_bedrock_converse/bedrock_runtime_client.ts
Outdated
Show resolved
Hide resolved
this.config.requestHandler = new NodeHttpHandler({ | ||
streaming: fields.streaming ?? true, | ||
actionsClient, | ||
connectorId, | ||
}); |
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.
@jacoblee93 finally got this working by overriding NodeHttpHandler.handle
. I'm able to hand back the stream without altering it. However, what tripped me up was non-streaming! It expected that to be encoded too. Both are working now, thanks for your help 🥳
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --update'
…o bedrock_converse
…o bedrock_converse
Pinging @elastic/security-solution (Team: SecuritySolution) |
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.
Changes to SIEM migrations LGTM. Thanks Steph
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.
response-ops
OK
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.
ResponseOps changes LGTM
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Page load bundle
History
|
Starting backport for target branches: 8.x |
(cherry picked from commit 755ef31)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…200817) # Backport This will backport the following commits from `main` to `8.x`: - [[Security solution] `ChatBedrockConverse` (#200042)](#200042) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Steph Milovic","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-19T20:45:31Z","message":"[Security solution] `ChatBedrockConverse` (#200042)","sha":"755ef312f2d533117ce3f614f8586e9c4db657be","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team: SecuritySolution","ci:cloud-deploy","Team:Security Generative AI","backport:version","v8.17.0"],"title":"[Security solution] `ChatBedrockConverse`","number":200042,"url":"https://github.com/elastic/kibana/pull/200042","mergeCommit":{"message":"[Security solution] `ChatBedrockConverse` (#200042)","sha":"755ef312f2d533117ce3f614f8586e9c4db657be"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200042","number":200042,"mergeCommit":{"message":"[Security solution] `ChatBedrockConverse` (#200042)","sha":"755ef312f2d533117ce3f614f8586e9c4db657be"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Steph Milovic <[email protected]>
Hi @MadameSheema, We have tested this ticket for the Amazon Bedrock Connector on the latest 8.17.0 SNAPSHOT build and below are our observations: Build details
Observations and screenshots
Please let us know if anything else is required to be added to the testing scenarios. Thank you! |
Summary
A couple of weeks ago, we updated our LangChain packages in both
8.x
andmain
branches (PR #198622). This update removed the code on LangChain's end in theBedrockChat
chat model that previously invoked non-streaming behavior when tools were provided. As a result, this change broke streaming in Bedrock for the Security Assistant.Updating from
BedrockChat
toChatBedrockConverse
The LangChain team recommends switching from the
BedrockChat
model to theChatBedrockConverse
model for the following reasons:ChatBedrockConverse
is developed and supported directly by LangChain, whileBedrockChat
was community-developed and is no longer a priority for LangChain.ChatBedrockConverse
.Given these points, and because fixing streaming in
BedrockChat
is non-trivial, we’ve decided to transition to theChatBedrockConverse
model.Changes in This PR
New Model Implementation:
ActionsClientChatBedrockConverse
model to support both streaming and non-streaming use cases..bedrock
stack_connector
to include two new sub-actions:converse
andconverseStream
. These correlate directly with the Amazon Bedrock APIs of the same names.Error Handling Updates:
ActionsClientBedrockChatModel
:ActionsClientBedrockChatModel
, they’ll receive a message instructing them to useActionsClientChatBedrockConverse
instead.Specific Use Case Updates:
integration_assistant
continues to useActionsClientBedrockChatModel
with streaming explicitly set tofalse
.siem_migrations
, I identified one spot where streaming could theoretically be set totrue
. However, it is currently alwaysfalse
. To avoid accidental misuse, I removed the parameter entirely.