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.

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] Elastic Assistant, OpenAI stream #169729

Merged
merged 70 commits into from
Nov 7, 2023

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Oct 24, 2023

Part 1 of streaming in Security's AI Assistant

Implements streaming for OpenAI in a non-langchain environment

To test:

  1. Have an OpenAI connector
  2. Update the feature flag to be true. Located in kbn-elastic-assistant/impl/assistant/api.tsx named isStream
  3. Test chatting with the AI Assistant. Test Stop generating button. Test Regenerate button

To do in follow up:

  1. Add support for langchain
  2. Bedrock streaming
  3. Remove isStream feature flag

@stephmilovic stephmilovic added release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Feature:GenAI labels Oct 24, 2023
Comment on lines +18 to +19
reader?: ReadableStreamDefaultReader<Uint8Array>;
content?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Is this interface intended to stay the same through part2? Will we support streaming turned on/off (even if just technically and not exposed via the UI)? If so, we might want to rework these types so one of the two are required. Does a message make sense without content now? I suppose the new empty 'loading' message would fit this interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if a message has both content + reader, we know to show the "Regenerate response" button. @jamesspi thinks we should turn streaming on 100% of the time. Even with streaming, once a reader is complete the message will get its content property

jest.clearAllMocks();
});

it('Should stream response. isLoading/isStreaming are true while streaming, isLoading/isStreaming are false when streaming completes', async () => {
Copy link
Contributor

@andrew-goldstein andrew-goldstein Nov 1, 2023

Choose a reason for hiding this comment

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

The description of this test appears to contain three assertions, and the implementation contains a total of 5 expects. Consider (generally) one assertion per test

Comment on lines +103 to +110
customCodeBlock: (props) => {
return (
<>
<CustomCodeBlock value={props.value} />
<EuiSpacer size="m" />
</>
);
},
Copy link
Member

Choose a reason for hiding this comment

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

Looks like everything around the markdown parsing/processing got moved over correctly, however I'm not seeing the codeblock actions show up anymore:

I confirmed the custom codeblock is rendering as expected, so it is probably something downstream w/ augmentMessageCodeBlocks

/**
* Augments the messages in a conversation with code block details, including
* the start and end indices of the code block in the message, the type of the
* code block, and the button to add the code block to the timeline.
*
* @param currentConversation
*/
export const augmentMessageCodeBlocks = (
currentConversation: Conversation
): CodeBlockDetails[][] => {
const cbd = currentConversation.messages.map(({ content }) =>
analyzeMarkdown(
getMessageContentWithReplacements({
messageContent: content ?? '',
replacements: currentConversation.replacements,
})
)
);

You had mentioned some changes around getMessageContentWithReplacements(), which this ends up calling -- perhaps it's coming from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've got this sort of figured out. First off, I dropped the className that the getControlContainer was looking for, className={'message-${index}'}. I put the class in place, and it is working again for NON-streaming.

Putting a debugger in the getControlContainer while streaming is turned on, when our comments changes from reader to content there seems to be a flash where the .euiCodeBlock__controls disappears. As soon as the debugger is played, the controls appear again. ❓

Playing with this, I tried switching from useLayoutEffect to useEffect, but the same issue happened. I added a setTimeout, and experimented with the timeout length and even a timeout of 0 seems to resolve the issue. Kind of a render bandaid that appears to be holding... what do you think? Any better ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline w/ Steph -- this all looks good and even improves the inconsistencies with the current experience, so all the better! 🙂

Copy link
Member

Choose a reason for hiding this comment

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

This might not be the relevant file (perhaps stream_observable), but commenting here instead of the main PR for thread support.

Anyway, I've somehow got myself in a state where all my replies end up being garbled as below.

While streaming vs. complete:

image image

Copy link
Member

Choose a reason for hiding this comment

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

I don't have exact repro steps for getting it consistently stuck, but at the time I was testing closing/opening the assistant mid-stream and then later opening/closing the timeline instance of the assistant while it was streaming as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shit ok, i've not seen this. I bet calling useStream's setComplete callback on modal close will prevent this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this should resolve the issue: cae753a

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks for this first phase of streaming @stephmilovic! 🙏
LGTM 🚀

@stephmilovic stephmilovic requested a review from a team as a code owner November 6, 2023 22:10
@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4662 4670 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/elastic-assistant 75 76 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 13.1MB 13.1MB +14.4KB
Unknown metric groups

API count

id before after diff
@kbn/elastic-assistant 95 96 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stephmilovic stephmilovic merged commit 2a52a36 into elastic:main Nov 7, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:GenAI release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants