Add SK chat support #47

milderhc · Oct 25, 2023

Purpose

Adding chat using Semantic Kernel.

Does this introduce a breaking change?

[ ] Yes
[x] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

Get the code

git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install

Test the code

Other Information

dantelmomsft

Can you rebase this with the new commits on the main. Furthermore make sure to synchronize with the last UI changes from john Oliver PR #44.

Other comments are inline.

app/backend/src/main/java/com/microsoft/openai/samples/rag/Application.java

...nai/samples/rag/chat/approaches/semantickernel/JavaSemanticKernelWithMemoryChatApproach.java

.../openai/samples/rag/chat/approaches/semantickernel/JavaSemanticKernelChainsChatApproach.java

dantelmomsft

PR looks good. 2 additional comments:

I think you lost chat.tsx UI changes when you merged the last commits from main on your branch.
Could you also add the streaming support

dantelmomsft · Oct 31, 2023

see this: f51e6f5#diff-506debba46b93087dc46a916384e56392808bcc02a99d9291557f3e674d4ad6c [https://avatars.githubusercontent.com/u/4893321?s=400&v=4]<https://github.com/Azure-Samples/azure-search-openai-demo-java/pull/47/commits/f51e6f57af4cf75a365b5c3e34c2b0d153d9668b#diff-506debba46b93087dc46a916384e56392808bcc02a99d9291557f3e674d4ad6c> Add SK chat support by milderhc · Pull Request #47 · Azure-Samples/azure-search-openai-demo-java<f51e6f5#diff-506debba46b93087dc46a916384e56392808bcc02a99d9291557f3e674d4ad6c> Purpose Adding chat using Semantic Kernel. Does this introduce a breaking change? [ ] Yes [x] No Pull Request Type What kind of change does this Pull Request introduce? [ ] Bugfix [x] Feature [... github.com

…

________________________________ From: Milder Hernandez ***@***.***> Sent: Tuesday, October 31, 2023 6:52 PM To: Azure-Samples/azure-search-openai-demo-java ***@***.***> Cc: Davide Antelmo ***@***.***>; Mention ***@***.***> Subject: Re: [Azure-Samples/azure-search-openai-demo-java] Add SK chat support (PR #47) Ignore that last request review. Could you point me to the first comment? I don't see this PR introducing any new changes to app/frontend/src/pages/chat/Chat.tsx. — Reply to this email directly, view it on GitHub<#47 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A2623W63ZVVBZ3I25PI2ZQ3YCE3GLAVCNFSM6AAAAAA6O7NTGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBXG4YDKNBYHA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

brunoborges · Nov 1, 2023

Hey @dantelmomsft , can you please review this carefuly?

milderhc · Nov 1, 2023

I added streaming to SK implementation, but as we don't support streaming on the SKFunctions, I'm returning the whole response in the runStreaming method. The only one to implement proper streaming at the moment would be using the same approach I initially added (57272e5), with String replacement to create chat instructions in the code, this is becuase OpenAIChatCompletion supports streaming.

Let me know if this is okay or would like to implement runStreaming the other way.

dantelmomsft

1 java unit tests is failing:

dantelmomsft · Nov 2, 2023

.../openai/samples/rag/chat/approaches/semantickernel/JavaSemanticKernelChainsChatApproach.java

+        String question = ChatGPTUtils.getLastUserQuestion(questionOrConversation.getMessages());
+
+        Kernel semanticKernel = buildSemanticKernel(options);
+


getSourcesFromCOnversation is missing. See what you've done here:
https://github.com/Azure-Samples/azure-search-openai-demo-java/pull/47/files#diff-b6597742f55dcf06e09eb19c789531ec2b2273fb3b56bd97fd929afbc451e5d5R77

Consider to refactor to one common method used both by the JavaSemanticKernelChainsChatApproach and JavaSemanticKernelWithMemoryChatApproach

get sources using extract keywords is still missing in the new commit. Please review the previous comment.

Thanks! Extracting keywords by using getSourcesFromConversation only applies to SK with memory approach, as it only uses semantic functions. SK chaining uses both, native and semantic functions, so I instead added a new native function to CognitiveSearchPlugin to search the keywords using that function. I moved the plugin to the retrieval package as well.

Let me know what you think

app/frontend/src/pages/chat/Chat.tsx

dantelmomsft · Nov 2, 2023

I added streaming to SK implementation, but as we don't support streaming on the SKFunctions, I'm returning the whole response in the runStreaming method. The only one to implement proper streaming at the moment would be using the same approach I initially added (57272e5), with String replacement to create chat instructions in the code, this is becuase OpenAIChatCompletion supports streaming.

Let me know if this is okay or would like to implement runStreaming the other way.

I was not aware java SK didn't support streaming.. in this case I would just leave it as "Not Implemented" for the time being. .@johnoliver WDYT ?

dantelmomsft

If you agree with the strategy I've shared in my last comment, we should at least add a dedicated production environment and deploy the tagged release. After that we can start use CI + scheduled daily build pipelines on the current dev environment. Please bring back the infra validation CI pipeline.

milderhc · Nov 6, 2023

@dantelmomsft I disabled streaming for SK approaches. I am also not sure if you meant the last comment for this PR.

.../openai/samples/rag/chat/approaches/semantickernel/JavaSemanticKernelChainsChatApproach.java

dantelmomsft · Nov 6, 2023

.../openai/samples/rag/chat/approaches/semantickernel/JavaSemanticKernelChainsChatApproach.java

+import java.util.stream.Collectors;
+
+/**
+ * Simple chat-read-retrieve-read java implementation, using the Cognitive Search and OpenAI APIs directly.


Please review the description looking at the the JavaSemanticKernelChainsApproach.
Please add this new approach to the implementations table in the README: https://github.com/Azure-Samples/azure-search-openai-demo-java/blob/main/README.md#rag-implementation-options

I'll update it once you agree with the last changes I added.

dantelmomsft · Nov 6, 2023

.../openai/samples/rag/chat/approaches/semantickernel/JavaSemanticKernelChainsChatApproach.java

+        String question = ChatGPTUtils.getLastUserQuestion(questionOrConversation.getMessages());
+
+        Kernel semanticKernel = buildSemanticKernel(options);
+


get sources using extract keywords is still missing in the new commit. Please review the previous comment.

...nai/samples/rag/chat/approaches/semantickernel/JavaSemanticKernelWithMemoryChatApproach.java

dantelmomsft · Nov 7, 2023

@milderhc PR approved. before merging can you make sure to update the README with the new approaches as mentioned in my last comment. thanks and great work.

johnoliver and others added 5 commits October 18, 2023 17:51

store wip

f51e6f5

Add SK chat support

57272e5

Merge branch 'main' into add-sk-chat

a7a23ba

Find sources based on conversation

f14456f

Remove unnecessary log

2667487

dantelmomsft self-requested a review October 27, 2023 09:58

dantelmomsft requested changes Oct 27, 2023

View reviewed changes

dantelmomsft assigned milderhc Oct 27, 2023

Milder Hernandez Cagua added 3 commits October 27, 2023 14:10

Convert to file plugins

d8d6b14

Remove cors configuration

e81cbc6

Add suggestions variable to AnswerConversation skill

dfb4e9b

milderhc marked this pull request as ready for review October 30, 2023 17:32

milderhc requested a review from brunoborges as a code owner October 30, 2023 17:32

dantelmomsft requested changes Oct 31, 2023

View reviewed changes

milderhc requested a review from dantelmomsft October 31, 2023 17:37

Merge 'main' into 'add-sk-chat'

319d617

brunoborges marked this pull request as draft November 1, 2023 17:01

Add streaming to sk chat

ee85ece

milderhc marked this pull request as ready for review November 1, 2023 19:04

milderhc force-pushed the add-sk-chat branch from 4d95ba2 to ee85ece Compare November 1, 2023 19:05

dantelmomsft requested changes Nov 2, 2023

View reviewed changes

Milder Hernandez Cagua added 3 commits November 2, 2023 18:37

Fix Chat.tsx

802b107

Fix RAGApproachFactorySpringBootImpl.createApproach

8219092

Disable streaming for SK

6866ede

milderhc requested a review from dantelmomsft November 6, 2023 06:53

dantelmomsft requested changes Nov 6, 2023

View reviewed changes

Add keywords extraction through native function for SK Chaining approach

3935ff1

dantelmomsft approved these changes Nov 7, 2023

View reviewed changes

Add new SK chat approaches to README.md

659b508

dantelmomsft merged commit ae612e9 into Azure-Samples:main Nov 8, 2023

		String question = ChatGPTUtils.getLastUserQuestion(questionOrConversation.getMessages());

		Kernel semanticKernel = buildSemanticKernel(options);

Search code, repositories, users, issues, pull requests...

Add SK chat support #47

Add SK chat support #47

Uh oh!

Conversation

milderhc commented Oct 25, 2023

Purpose

Does this introduce a breaking change?

Pull Request Type

How to Test

Other Information

Uh oh!

dantelmomsft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dantelmomsft left a comment

Choose a reason for hiding this comment

Uh oh!

dantelmomsft commented Oct 31, 2023 via email

Uh oh!

brunoborges commented Nov 1, 2023

Uh oh!

milderhc commented Nov 1, 2023

Uh oh!

dantelmomsft left a comment

Choose a reason for hiding this comment

Uh oh!

dantelmomsft Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

dantelmomsft Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milderhc Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dantelmomsft commented Nov 2, 2023

Uh oh!

dantelmomsft left a comment

Choose a reason for hiding this comment

Uh oh!

milderhc commented Nov 6, 2023

Uh oh!

Uh oh!

dantelmomsft Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

milderhc Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

dantelmomsft Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dantelmomsft commented Nov 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dantelmomsft Nov 6, 2023 •

edited

Loading

milderhc Nov 7, 2023 •

edited

Loading

dantelmomsft Nov 6, 2023 •

edited

Loading