[Security Assistant] Abort signal fix #203041
Merged
+38
−23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephmilovic
added
release_note:skip
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
stephmilovic
commented
Dec 5, 2024
| @@ -160,10 +160,7 @@ export const streamGraph = async ({ | |||
| finalMessage += msg.content; | |||
| } | |||
| } else if (event.event === 'on_llm_end' && !didEnd) { | |||
| const generations = event.data.output?.generations[0]; | |||
| if (generations && generations[0]?.generationInfo.finish_reason === 'stop') { | |||
There was a problem hiding this comment.
this condition prevented aborted generations from being persisted in the conversation
KDKHD
approved these changes
Dec 5, 2024
|
Starting backport for target branches: 8.15, 8.16, 8.17, 8.x |
💚 Build Succeeded
Metrics [docs]
History
|
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 5, 2024
(cherry picked from commit b3b2c17)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 5, 2024
(cherry picked from commit b3b2c17)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 5, 2024
(cherry picked from commit b3b2c17)
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
stephmilovic
removed
the
backport:prev-major
stephmilovic
added
backport:prev-minor
|
Starting backport for target branches: 8.x |
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 5, 2024
(cherry picked from commit b3b2c17)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 5, 2024
(cherry picked from commit b3b2c17)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Dec 6, 2024
# Backport This will backport the following commits from `main` to `8.x`: - [[Security Assistant] Abort signal fix (#203041)](#203041) <!--- 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-12-05T21:27:29Z","message":"[Security Assistant] Abort signal fix (#203041)","sha":"b3b2c1745ac846b377d8e234526aa965196e4de9","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team: SecuritySolution","backport:prev-major","Team:Security Generative AI"],"title":"[Security Assistant] Abort signal fix","number":203041,"url":"https://github.com/elastic/kibana/pull/203041","mergeCommit":{"message":"[Security Assistant] Abort signal fix (#203041)","sha":"b3b2c1745ac846b377d8e234526aa965196e4de9"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203041","number":203041,"mergeCommit":{"message":"[Security Assistant] Abort signal fix (#203041)","sha":"b3b2c1745ac846b377d8e234526aa965196e4de9"}}]}] BACKPORT--> Co-authored-by: Steph Milovic <[email protected]>
markov00
pushed a commit
to markov00/kibana
that referenced
this pull request
Dec 7, 2024
SoniaSanzV
pushed a commit
to SoniaSanzV/kibana
that referenced
this pull request
Dec 9, 2024
SoniaSanzV
pushed a commit
to SoniaSanzV/kibana
that referenced
this pull request
Dec 9, 2024
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Dec 9, 2024
Samiul-TheSoccerFan
pushed a commit
to Samiul-TheSoccerFan/kibana
that referenced
this pull request
Dec 10, 2024
mykolaharmash
pushed a commit
to mykolaharmash/kibana
that referenced
this pull request
Dec 11, 2024
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Dec 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
I noticed that
abortSignalwas not being properly piped through for any of the connector types. I suspect this happened when we made the switch to be fully LangChain. It is most evident with OpenAI, as the stream never finishes (handleStreamEndnot called) so the message does not get persisted, resulting in the following misbehavior:bad1.mov
In Bedrock and Gemini we had the
handleStreamEndbehavior correctly handling abort signal. However, we did not pass through the signal in the chat model. On top of that, for some reason Bedrock gets the abort signal off of theinvokeconfig and we were not providingsignalthere. I made changes to pass the signal to the connector, and to pass the signal through theinvokeconfig for Bedrock.Impact
OpenAI
OpenAI had the biggest impact. Below is a screen capture of the problematic steps above, now behaving as expected. The stream runs slightly longer on the server, so when the UI updates it has a little extra text than when the stream originally finished.
good1a.mov
Bedrock
Bedrock behaves differently in that when you end the stream, it still gives you the complete output. So although the signal is now included in the connector request, there is not a UI implication. It behaves the same way as before: The stream will stop in the UI, but when you reload the conversation the full response will be there.
Gemini
The stream is so fast in Gemini, I can't actually successfully press "Stop generating" so it seems there is no user impact. However, I ensured that the signal is now included in the connector request.