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

.Net: GetFunctionResponse doesn't work with IChatStreamingResult (and can't) #3198

Closed
stephentoub opened this issue Oct 16, 2023 · 0 comments · Fixed by #3246
Closed

.Net: GetFunctionResponse doesn't work with IChatStreamingResult (and can't) #3198

stephentoub opened this issue Oct 16, 2023 · 0 comments · Fixed by #3246
Assignees
Labels
.NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)
Milestone

Comments

@stephentoub
Copy link
Member

IChatResult has a GetFunctionResponse extension method that extracts the function response information from the result. IChatStreamingResult inherits IChatResult, but GetFunctionResponse doesn't work for it, and can't: the function information sent down as part of a function response can (and almost always does) span multiple streaming results, e.g. the first result might only say it's a function call, the second might only contain the name, and the arguments might be split across the third, fourth, fifth, etc.

@shawncal shawncal added .NET Issue or Pull requests regarding .NET code triage labels Oct 16, 2023
@evchaki evchaki added the sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community) label Oct 19, 2023
@evchaki evchaki added this to the v1.0.0 milestone Oct 19, 2023
github-merge-queue bot pushed a commit that referenced this issue Oct 20, 2023
### Motivation and Context

It is easy to mistakenly call the non-streaming API of the streaming
interface when you want streaming.
This also resolves problem where non streaming behavior/implementation
is enforced upon streaming objects.

Callers should be driven to the "pit of success" of calling the intended
API.

Resolves #2730 
Resolves Partially #3198

### Description

Removes the inheritance from ITextResult and IChatResult from streaming
counterparts.

This will avoid Non-streaming objects to be derived as `Streaming
capable` objects.

### Contribution Checklist

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this issue Nov 1, 2023
…oft#3230)

### Motivation and Context

It is easy to mistakenly call the non-streaming API of the streaming
interface when you want streaming.
This also resolves problem where non streaming behavior/implementation
is enforced upon streaming objects.

Callers should be driven to the "pit of success" of calling the intended
API.

Resolves microsoft#2730 
Resolves Partially microsoft#3198

### Description

Removes the inheritance from ITextResult and IChatResult from streaming
counterparts.

This will avoid Non-streaming objects to be derived as `Streaming
capable` objects.

### Contribution Checklist

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄
github-merge-queue bot pushed a commit that referenced this issue Nov 1, 2023
)

### Motivation and Context

Resolves #3198 

Today we can't call functions using streaming, this change allows it to
be used.

The current approach will require the stream to be buffered while
listening to a potential function call.

The example shows how that will be achieved.
```
StringBuilder chatContent = new();

// Non function result streaming will happen here.
await foreach (var message in chatResult.GetStreamingChatMessageAsync())
{
    if (message.Content is not null)
    {
        Console.Write(message.Content);
        chatContent.Append(message.Content);
    }
}
chatHistory.AddAssistantMessage(chatContent.ToString());

// After all stream was loaded check if a `FunctionResponse` was present.
var functionResponse = await chatResult.GetStreamingFunctionResponseAsync();
```

Remark: Calling `GetStreamingFunctionResponseAsync()` will buffer the
stream to capture the full function call definition, use it after all
the streaming messages were flushed out.

### Description

Allows streaming interfaces to get the function result.

This is a simplified approach (Buffer the streaming of a function call
request until it's complete). Using the FunctionCall

### Contribution Checklist

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants