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

Implement IChatClient #111

Merged
merged 28 commits into from
Oct 26, 2024
Merged

Implement IChatClient #111

merged 28 commits into from
Oct 26, 2024

Conversation

awaescher
Copy link
Owner

@awaescher awaescher commented Oct 17, 2024

@awaescher awaescher added the breaking changes Might introduce or introduces breaking changes label Oct 17, 2024
@awaescher awaescher added the help wanted Extra attention is needed label Oct 17, 2024
for the records, I am not a fan of this, see #81
Properties = functionMetadata.Parameters.ToDictionary(p => p.Name, p => new Properties
{
Description = p.Description,
Enum = [], // TODO is there such as possible values in AIFunctionParameterMetadata?
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use something like Enum.GetValues(p.ParameterType)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if you want to be more precise about it, it's possible to use p.Schema. If it's a JsonElement, it's telling you the JSON schema, so you could use that to read the set of possible values. That would be more technically correct since it would work regardless of AOT and trimming concerns. However if you want to start with Enum.GetValues(p.ParameterType) that would be a reasonable starting point.

Copy link
Owner Author

@awaescher awaescher Oct 21, 2024

Choose a reason for hiding this comment

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

Thats a nice idea but where can I find how to handle it? What can I check for and what are the possible return values? Got me any resource or a way how to test it?

private static string ToFunctionTypeString(JsonObject? schema)
{
  return "string"; // TODO others supported?
}

I'll close the other comments aiming for the same change with this note that the JsonSchema approach would be good for these methods:

  • GetPossibleValues()
  • ToFunctionTypeString()

@SteveSandersonMS
Copy link
Contributor

@awaescher This looks superb to me!

@RogerBarreto
Copy link
Contributor

Important

Strongly suggest considering this change to be available in a feature-branch or be a pre-release package

Currently Semantic Kernel cannot have a dependency on .Net 9 features yet, if any updates after this merges into main we won't be able to update to your connector until it becomes GA, assume any other customers that also have a hard (.Net 8 LTS) configuration will be in a same position.

SK is also adding support for IChatClient clients in a feature branch waiting the right timing to be publicly available.

@SteveSandersonMS
Copy link
Contributor

SteveSandersonMS commented Oct 19, 2024

I'm not sure this is actually a problem. This change doesn't entail taking a dependency on .NET 9.

Microsoft.Extensions.AI.Abstractions is netstandard2.0 and is an out-of-band package - it's not part of the .NET 9 release. The only consequential dependency it involves is System.Text.Json, and that will be version 8, which (very soon) will be the oldest version that is still under support anyway.

@RogerBarret0
Copy link

@SteveSandersonMS Thanks for the added context, would like to follow more on that info on the System.Text.Json.

@awaescher awaescher removed the help wanted Extra attention is needed label Oct 21, 2024
@awaescher awaescher marked this pull request as ready for review October 21, 2024 19:56
@awaescher
Copy link
Owner Author

You'll see this change as soon as the next preview version of the package is published (by default, this happens monthly).

Awesome. Can we get this earlier, so that I could merge and build 4.0?

@RogerBarreto: Semantic Kernel could stay at 3.x after your pull request #114 but should also be able update to 4.x right?

@awaescher
Copy link
Owner Author

Published another preview version 4.0.0-preview.9 containing the latest changes from this pull-request.

@RogerBarreto
Copy link
Contributor

@RogerBarreto: Semantic Kernel could stay at 3.x after your pull request #114 but should also be able update to 4.x right?

You are right, we will update to the 4.0 as soon this feature branch

microsoft/semantic-kernel#9183

Is merged in main

@awaescher
Copy link
Owner Author

I guess I'll build a 4.0 as soon as Microsoft.Extensions.AI.Abstractions uses System.Text.Json in version 8.0.5.

@stephentoub
Copy link
Contributor

I guess I'll build a 4.0 as soon as Microsoft.Extensions.AI.Abstractions uses System.Text.Json in version 8.0.5.

A new version is up on nuget.

@awaescher awaescher merged commit ae92d85 into main Oct 26, 2024
1 check passed
@awaescher
Copy link
Owner Author

Awesome, thanks @stephentoub.
So we got 4.0 out 🎉

@awaescher awaescher deleted the feature/ichatclient branch October 26, 2024 19:53
@awaescher awaescher mentioned this pull request Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Might introduce or introduces breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants