-
Notifications
You must be signed in to change notification settings - Fork 30
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
Update ChatBot to work with dotnet-isolated and remove durable dependency #10
Conversation
take latest of main and merge to your current branch, should have some conflicts and would require some changes |
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.
Initial feedback on the samples and worker extension changes. Haven't looked at the WebJobs changes yet.
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/TextCompletions.cs
Outdated
Show resolved
Hide resolved
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
src/Functions.Worker.Extensions.OpenAI/Functions.Worker.Extensions.OpenAI.csproj
Outdated
Show resolved
Hide resolved
src/Functions.Worker.Extensions.OpenAI/TextCompletionInputAttribute.cs
Outdated
Show resolved
Hide resolved
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/host.json
Outdated
Show resolved
Hide resolved
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
src/Functions.Worker.Extensions.OpenAI/Functions.Worker.Extensions.OpenAI.csproj
Outdated
Show resolved
Hide resolved
src/Functions.Worker.Extensions.OpenAI/TextCompletionInputAttribute.cs
Outdated
Show resolved
Hide resolved
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.
Added more feedback, this time including the table storage implementation.
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI/Agents/ChatBotBindingConverter.cs
Outdated
Show resolved
Hide resolved
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.
More comments, this time a bit more skewed towards code style and organization. We to make sure the changes in your PR are consistent with the rest of the code in this repo.
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
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.
A few more comments, mostly around code style but a few around proper API usage, and one about potential issues with batch transaction sizes. I think we're nearly done!
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
some changes in https://github.com/Azure/azure-functions-openai-extension/blob/main/build/build-release.yml to publish worker nupkg as well can be done once worker reference is updated Line 54 - Pattern: WebJobs.Extensions.OpenAI*.dll The attached build should produce 3 nupkg in artifacts |
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.
Just a few additional tiny suggestions plus one small question, but all blockers are resolved so I'm signing off. :)
samples/other/dotnet/csharp-ooproc/CSharpIsolatedSamples/ChatBotIsolated.cs
Outdated
Show resolved
Hide resolved
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.
approving the PR, no blocking comment, check the build issues
Resolves issue: https://github.com/Azure/azure-functions-pyfx-planning/issues/141
This PR removes the durable dependency in the chat bot as that was causing issues in the compatibility with dotnet-isolated since durable can't run a built in function that is technically OOP in in-proc mode. We decided to replace durable with table storage, in which the
ChatBotStateEntity
stores the state of the chatbot and theChatMessageTableEntity
represents each message that the chat bot processes. Each entity has the partition key being the id of the chat bot and the rowkey is calledChatMessage
+ index number of that message.The table storage is configured by the user by adding the following in the host.json:
StorageConnectionName
can be any connection string of a storage account andCollectionName
is the name of the table that would hold the chat bot state and messages.Here is an example of what the azure storage table will look like:
ChatBotState
represents the state of the chatbot, and the corresponding messages are all of typeChatMessageEntity
which contains each user that is relayed to and by the chatbot.