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

TextCompletion Fixes + Interface simplification #1391

Merged

Conversation

RogerBarreto
Copy link
Member

Motivation and Context

Previous interfaces where too verbose and could be confusing.
Removed the extra Completion verbosity from the interfaces as marked the old ones as Obsolete.

Small bug identified when using the TextCompletion Streaming (awaiting indefinitely)

Description

  • Renamed interface ITextCompletionResult to ITextResult
  • Renamed interface ITextCompletionStreamingResult to ITextStreamingResult
  • BugFix TextCompletion Streaming (was not behaving as expected - awaiting indefinitely for ITextStreamingResult)
  • Added remarks on the Complete Extension methods regarding no support for multiple results.
  • Added remarks on the CompleteStreaming Extension methods regarding no support for multiple results.
  • Added remarks to extensions methods that don't support multiple results
    • IChatCompletion.GenerateMessageStreamAsync
    • IChatCompletion.GenerateMessageAsync
    • ITextCompletion.CompleteStreamAsync
    • ITextCompletion.CompleteAsync

Contribution Checklist

@RogerBarreto RogerBarreto added bug Something isn't working PR: ready for review All feedback addressed, ready for reviews .NET Issue or Pull requests regarding .NET code samples labels Jun 9, 2023
@RogerBarreto RogerBarreto self-assigned this Jun 9, 2023
@github-actions github-actions bot added the kernel Issues or pull requests impacting the core kernel label Jun 9, 2023
@RogerBarreto RogerBarreto added PR: ready to merge PR has been approved by all reviewers, and is ready to merge. and removed PR: ready for review All feedback addressed, ready for reviews labels Jun 13, 2023
Copy link
Member

@lemillermicrosoft lemillermicrosoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, since this might break anyone who was using these interfaces directly, let's bump the version number.

@lemillermicrosoft lemillermicrosoft added PR: ready for review All feedback addressed, ready for reviews PR: ready to merge PR has been approved by all reviewers, and is ready to merge. labels Jun 13, 2023
@shawncal shawncal merged commit 7e55f20 into microsoft:main Jun 15, 2023
@lemillermicrosoft lemillermicrosoft added this to the Sprint 33 milestone Jun 20, 2023
shawncal added a commit to shawncal/semantic-kernel that referenced this pull request Jul 6, 2023
### Motivation and Context
Previous interfaces were too verbose and could be confusing. 
Removed the extra `Completion` verbosity from the interfaces as marked
the old ones as Obsolete.

Small bug identified when using the TextCompletion Streaming (awaiting
indefinitely)

### Description

- Renamed interface `ITextCompletionResult` to `ITextResult`
- Renamed interface `ITextCompletionStreamingResult` to
`ITextStreamingResult`
- BugFix TextCompletion Streaming (was not behaving as expected -
awaiting indefinitely for ITextStreamingResult)
- Added remarks on the `Complete` Extension methods regarding no support
for multiple results.
- Added remarks on the `CompleteStreaming` Extension methods regarding
no support for multiple results.
- Added remarks to extensions methods that don't support multiple
results
  - IChatCompletion.GenerateMessageStreamAsync
  - IChatCompletion.GenerateMessageAsync
  - ITextCompletion.CompleteStreamAsync
  - ITextCompletion.CompleteAsync

Co-authored-by: Lee Miller <lemiller@microsoft.com>
Co-authored-by: Shawn Callegari <36091529+shawncal@users.noreply.github.com>
@RogerBarreto RogerBarreto deleted the features/itextcompletion-simpleinterfaces branch December 5, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code PR: ready for review All feedback addressed, ready for reviews PR: ready to merge PR has been approved by all reviewers, and is ready to merge.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants