Azure-Samples / azure-search-openai-demo-java

This repo is the Java version of Microsoft's sample app for ChatGPT + Enterprise data.
MIT License
67 stars 66 forks source link

Add SK chat support #47

Closed milderhc closed 7 months ago

milderhc commented 8 months ago

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

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

Other Information

dantelmomsft commented 7 months ago

see this: https://github.com/Azure-Samples/azure-search-openai-demo-java/pull/47/commits/f51e6f57af4cf75a365b5c3e34c2b0d153d9668b#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-javahttps://github.com/Azure-Samples/azure-search-openai-demo-java/pull/47/commits/f51e6f57af4cf75a365b5c3e34c2b0d153d9668b#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 GitHubhttps://github.com/Azure-Samples/azure-search-openai-demo-java/pull/47#issuecomment-1787705488, or unsubscribehttps://github.com/notifications/unsubscribe-auth/A2623W63ZVVBZ3I25PI2ZQ3YCE3GLAVCNFSM6AAAAAA6O7NTGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOBXG4YDKNBYHA. You are receiving this because you were mentioned.Message ID: @.***>

brunoborges commented 7 months ago

Hey @dantelmomsft , can you please review this carefuly?

milderhc commented 7 months ago

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 (https://github.com/Azure-Samples/azure-search-openai-demo-java/pull/47/commits/57272e566c988ba541a28e514038e568d597311f), 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 commented 7 months ago

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 ?

milderhc commented 7 months ago

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

dantelmomsft commented 7 months ago

@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.