-
Notifications
You must be signed in to change notification settings - Fork 428
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
Add streaming methods for Conversation class #74
Add streaming methods for Conversation class #74
Conversation
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.
As i'm not the owner, but just saw this project today and wanted to contribute here's my 50cents...
OpenAI_API/Chat/Conversation.cs
Outdated
/// If you are on the latest C# supporting async enumerables, you may prefer the cleaner syntax of <see cref="StreamResponseEnumerableFromChatbot"/> instead. | ||
/// </summary> | ||
/// <param name="resultHandler">An action to be called as each new result arrives.</param> | ||
public async Task StreamResponseFromChatbot(Action<string> resultHandler) |
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.
Please use suffix Async
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.
Conversation
's async Task<string> GetResponseFromChatbot()
doesn't have Async
suffix. But some async methods in other classes have. So I am a little bit confused.
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.
The majority of the codebase uses Async suffix except for the tests, but you're right aswell. One of the questions i still had when looking through the code :)
Per Microsoft Async suffix should not be used unless you have synchronous AND asynchronous methods for the same functionality. But to not exclude synchronous or create ambiguity it's best practice to suffix it with Async
to always have the option open. But yeah it should be consistent heheh. #71 will help with this i guess
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.
Sorry, I guess I missed one
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.
I added "Async" suffix
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.
Conversation
class already has a GetResponseFromChatbot()
method. So I name the stream methods StreamResponseFromChatbot
and StreamResponseEnumerableFromChatbot
.
With Async
suffix it will be like StreamResponseEnumerableFromChatbotAsync
, which seems too long.
In CompletionEndpoint
we have CreateCompletionAsync()
and StreamCompletionAsync()
. So maybe this is better?
GetResponseAsync()
StreamResponseAsync()
StreamResponseEnumerableAsync()
Can I modify the name of GetResponseFromChatbot()
in this pull request?
if (res.Choices.FirstOrDefault()?.Delta is ChatMessage delta) | ||
{ | ||
responseRole = delta.Role; | ||
string deltaContent = delta.Content; |
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.
ChatMessage.Content
is a string so you can use var
here instead of the alias string
for the variable
req.Messages = _Messages.ToList(); | ||
|
||
StringBuilder responseStringBuilder = new StringBuilder(); | ||
ChatMessageRole responseRole = null; |
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.
C# 8 comes with NRT, please use it appropiatly: ChatMessageRole? responseRole = null;
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.
I wanted to do so, but nullable
is not enabled for this project
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.
Hmm without the nullable feature you can say: #nullable enable
around your methods/classes.
The NRT feature mostly enables static analysis in this case.. You clearly assign null
to the variable so why not explicitly state so? This way intent is shown and it's clear that this variable is indeed null at start.
When will it be merged into a release version? |
I'm sorry about the inconsistent naming with As for nullable, I'm trying to keep this as backward-compatible as possible, working on .NET Standard and .NET framework as well. I don't think that can be done if I enable nullable? To be honest .NET has been changing so fast, I'm not really sure about this new nullable stuff. |
Thanks @JasonWei512, this is a much better implementation of streaming results from chat! |
Add streaming methods for
Conversation
class for convenience.Using the new C# 8.0 async iterators:
Or if using classic .NET framework or C# <8.0: