Add Microsoft.Extensions.AI support

[stephentoub]

Fixes #4

This provides implementations of IChatClient for both the main and beta clients. The actual src additions are a few hundred lines; most of the PR is extensive tests covering using the anthropic clients via IChatClient, providing coverage of both.

[sd-st]

Hey, thanks for the PR!

I left some comments; I'm not super familiar with Microsoft.Extensions.AI so some comments are probably a little silly. Overall though this looks really good, just had a few minor points.

Aside from the comments:

• We should probably add this to the README, and also put an example for it in examples
• Some lints are failing (You can check this with ./scripts/lint)

[stephentoub]

@sd-st, I just merged in the latest in main, and there are a few changes that are quite cumbersome. In particular, making all of the properties on MessageCreateParams be init is really hard to work with. Take a look at the GetMessageCreateParams in this PR and consider the changes that I'm now having to make in order for this to work. I'm going to do so, but please reconsider having these be init-only.

[stephentoub]

Actually, @sd-st, when you combine the init-only aspect with the fact that all of the init's always set a value for the specified property even when storing null, it's basically impossible to now do the right thing, as you can't conditionally set an init.

[stephentoub]

Looks like I can work around it for now by just using the raw properties and bypassing the strongly-typed API altogether...

[TomerAberbach]

I'll let Stephen take a look tomorrow and give his opinion as well, but I think:

• In most cases the user can compute the necessary info/data before/in subroutines and pass those results when constructing the immutable data model
• In some cases it may be appropriate to use with expressions to create new instances
• In a limited number of ultra dynamic cases, using the raw properties may be appropriate

I've not looked deeply at this PR so not sure which one applies, but just wanted to note that

[stephentoub]

In most cases the user can compute the necessary info/data before/in subroutines and pass those results when constructing the immutable data model

They can, and when it's just init in the mix, you can often make it work. But it's compounded by how these implementations currently work. Consider for example MessageCreateParams.Temperature:

    public double? Temperature
    {
        get
        {
            if (!this._bodyProperties.TryGetValue("temperature", out JsonElement element))
                return null;

            return JsonSerializer.Deserialize<double?>(element, ModelBase.SerializerOptions);
        }
        init
        {
            this._bodyProperties["temperature"] = JsonSerializer.SerializeToElement(
                value,
                ModelBase.SerializerOptions
            );
        }
    }

It's nullable, but if you set Temperature = null;, that's still going to write a JsonElement into this._bodyProperties["temperature"]. Because the property is init, if I want to write a routine like:

public static MessageCreateParams Create(double? temperature)
{
   ...
}

I can't implement it like this because of the init:

public static MessageCreateParams Create(double? temperature)
{
    MessageCreateParams p = new();
    if (temperature is not null)
    {
        p.Temperature = temperature;
    }
    return p;
}

and I can't write it like this because doing so populates the MessageCreateParams with an empty temperature, which will result in "temperature":null being emitted in the JSON when it's serialized:

public static MessageCreateParams Create(double? temperature)
{
    return new()
    {
        Temperature = temperature,
    };
}

which means my option is to write it like:

public static MessageCreateParams Create(double? temperature)
{
    return temperature is not null ?
        new()
        {
            Temperature = temperature,
        } :
        new();
}

That's fine if I just have the one property to set. But if I have two, now I have four possible ways I need to construct the instance. Three properties, eight. Etc. It's exponential.

I could, as you suggest, with these now being records, use with, but that ends up creating a whole new object and subdictionaries and so on for every property that gets set (since each would be done one at a time). Maybe the overhead wouldn't be prohibitive, especially given all the other expenses involved currently with roundtripping to JSON for each get/set, but it's an unfortunate expense, and O(N^2) allocation in terms of the number of properties set (given the dictionaries involved).

I pushed a commit that drops down to just working with the raw JSON. But from a usability perspective, it's quite poor.

Separate from this PR and just for general usability of the library, I'd urge reconsidering init and/or making the codegen more amenable to nulls being stored. If I could store null and have it be a nop when already null, then I could just have a shadow local for every property and construct the instance once with all of its properties set to those locals. I can't do that now, though, due to the aforementioned issue of setting null not being a nop.

[TomerAberbach]

Ah, hmmm, for properties that can only be omitted or set to non-null, I think we should turn null init into omitted. For properties that can be both null or omitted, we can't do that

I'll need to double check the codegen implementation tomorrow to see if that's what we're currently doing (can't tell from this PR cause I'm not sure off the top of my head what the schema of each property is). If it's not what we're doing, then I'll fix that, which should help in most cases (it's rare-ish for an API to have a property that can be both null or omitted)

[sd-st]

@stephentoub FYI many of the above improvements we discussed (including init for non-required properties) have been put into next, so if you rebase off of that branch you should be good!

[stephentoub]

Thanks, @sd-st. Updated and rebased on next.

[sd-st]

Aside from the comments, there are some failing lints (./scripts/lint) and it probably makes sense to add something about this to the readme and examples

[stephentoub]

Thanks, @sd-st. All feedback addressed and merged with latest next.

[stephentoub]

Ah, I missed your comment about linting errors... where can I see them?

[sd-st]

Ah, I missed your comment about linting errors... where can I see them?

If you run ./scripts/lint you should be able to see them

[stephentoub]

Ok, done. Thanks.

[stephentoub]

@sd-st, I've rebased back on main now that it's been updated with everything from next.

Anything else I can do to help push this forward?

Thanks!

[sd-st]

Hey @stephentoub - sorry about the long delay here. Haven't forgotten about this, just have some fairly high pri stuff on my plate. I'm hoping to get to this over the next few days but I'm off on Friday so I can't make any promises.

[stephentoub]

Sounds good. Thanks.

[sd-st]

FYI, we will probably still want to merge this into next I think

[stephentoub]

FYI, we will probably still want to merge this into next I think

Ok :) Rebased back on next.

[sd-st]

Looks pretty good! Just had a few minor comments. As an aside, the formatting looks a tiny bit weird in some places - I suspect a ./scripts/format should clean things up

stale

[sd-st]

@stephentoub I got a pretty big diff from ./scripts/format. It looks like a lot of that is unrelated to this PR, but if you could autoformat the changed files with dotnet csharpier format path/to/file that would be appreciated. LGTM other than that!

[stephentoub]

@stephentoub I got a pretty big diff from ./scripts/format. It looks like a lot of that is unrelated to this PR, but if you could autoformat the changed files with dotnet csharpier format path/to/file that would be appreciated. LGTM other than that!

Done, thanks.

[sd-st]

@stephentoub Hey, really sorry about all the merge conflicts. If you want to do one more conflict fix + formatting pass (if it needs it) I'll be standing by to give it a quick review + merge. Excited to get this in (:

[sd-st]

Yay! Thank you for this PR (:

[stephentoub]

Thank you, @sd-st.