Add AddAzureOpenAIChatClient #6225

SteveSandersonMS · 2024-10-10T08:43:19Z

Description

This is a starting point based on the discussion earlier this week with @eerhardt, @sebastienros, and @luisquintanilla.

It allows developers to register an IChatClient in client projects via:

builder.AddAzureOpenAIChatClient("openai");

// Or with custom pipeline:
builder.AddAzureOpenAIChatClient("openai", pipeline => pipeline
    .UseChatOptions(...)
    .UseFunctionCalling());

So far I only did this for Azure OpenAI but once we've agreed on the shape would do the equivalent for OpenAI too.

Design questions

Lifetime

Currently this registers the IChatClient as singleton. That differs from prior assumptions in the MEAI repo that it would be scoped (cc: @stephentoub). The reason I did singleton here is that the underlying OpenAIClient is singleton, and being singleton and thread-safe is cited as a core tenet of Azure client libraries.

I suspect we should try to create an expectation that IChatClient implementations should be designed to be singleton and thread-safe. Anyone who finds they need per-usage-site state can store it in ChatOptions (e.g., AdditionalProperties). If this turns out to be impractical then we will have to reconsider this design. But it's better to start with singleton and later relax it to be scoped than to go in the other direction.

Also filed dotnet/extensions#5499 to track on the MEAI side validating this for the existing IChatClient implementations.

If I'm misjudging this and Aspire is totally OK with client libraries registering as scoped, we can reconsider.

APIs for chaining on `AddAzureOpenAIClient`

It's also possible we might want to enable an API pattern like this:

builder.AddAzureOpenAIClient("openai")
    .AddChatClient()          // Optionally also pass model/deployment or we'll get it from config
    .AddEmbeddingGenerator(); // Likewise

... instead of requiring two top-level calls (AddAzureOpenAIChatClient and AddAzureOpenAIEmbeddingGenerator). However that would involve changing the return type of AddAzureOpenAIClient from void to some builder we can put AddChatClient and AddEmbeddingGenerator extensions onto.

I'm fine with doing this if we like it. There's an argument for still also supporting the top-level calls for equivalence with AddOllamaChatClient etc.

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No but will do when API shape is agreed
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No (justification: it's just registering DI services, which the developer would otherwise do manually and get the same end result)
Does the change require an update in our Aspire docs?
- Yes
  - Link to aspire-docs issue: We need to confirm the API first
- No

Microsoft Reviewers: Open in CodeFlow

SteveSandersonMS · 2024-10-10T08:59:25Z

BTW does anyone know why Microsoft.Extensions.AI is not on the dotnet-public feed at https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json? Since prerelease builds of M.E.DI.Abstractions are there, I'd expect M.E.AI to be there too.

stephentoub · 2024-10-10T13:26:45Z

Currently this registers the IChatClient as singleton.

Thank you 😄

stephentoub · 2024-10-10T13:27:07Z

BTW does anyone know why Microsoft.Extensions.AI is not on the dotnet-public feed at https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json? Since prerelease builds of M.E.DI.Abstractions are there, I'd expect M.E.AI to be there too.

Most likely no one requested it to be mirrored there. I can do so. But for future reference: https://github.com/dotnet/arcade/blob/main/Documentation/MirroringPackages.md

sebastienros · 2024-10-10T21:29:10Z

@stephentoub, "/azp run"?

stephentoub · 2024-10-10T21:30:27Z

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

+    {
+        builder.AddAzureOpenAIClient(connectionName, configureSettings, configureClientBuilder);
+
+        builder.Services.AddSingleton(services =>


I assume if we change AddChatClient to be a singleton, this would instead use that?

Yes, it would be equivalent, so it might as well do.

stephentoub · 2024-10-10T21:31:03Z

@stephentoub, "/azp run"?

Possibly. I'm used to that not being appropriate in dotnet/runtime.

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

stephentoub · 2024-10-10T21:37:36Z

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

+        if (configuration.GetConnectionString(connectionName) is string connectionString)
+        {
+            var connectionBuilder = new DbConnectionStringBuilder { ConnectionString = connectionString };
+            deploymentName = (connectionBuilder[DeployentKey] ?? connectionBuilder[ModelKey])?.ToString();


It looks like DbConnectionStringBuilder's indexer actually throws rather than returning null if the keyword isn't found. Should these instead use TryGetValue?

https://github.com/dotnet/runtime/blob/e32148a8bd322f0b7cd5368d4c3810354d9fc14a/src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionStringBuilder.cs#L78-L90

Example: we use connectionBuilder.TryGetValue(ConnectionStringEndpoint, out var endpoint) in AddOpenAIClientFromConfiguration.

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

sebastienros · 2024-10-10T21:42:04Z

I think the builder alternative was suggested instead of a lambda in the extension method because we already have two optional lambdas in it (to customize option and settings).

    public static void AddAzureOpenAIClient(
        this IHostApplicationBuilder builder,
        string connectionName,
        Action<AzureOpenAISettings>? configureSettings = null,
        Action<IAzureClientBuilder<AzureOpenAIClient, AzureOpenAIClientOptions>>? configureClientBuilder = null)

stephentoub

Design looks reasonable to me.

sebastienros · 2024-10-10T21:52:31Z

src/Components/Aspire.OpenAI/MEAIPackageOverrides.targets

+    <PackageReference Include="Microsoft.Extensions.Primitives" VersionOverride="9.0.0-*" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" VersionOverride="9.0.0-*" />
+    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" VersionOverride="9.0.0-*" />
+    <PackageReference Include="System.Text.Json" VersionOverride="9.0.0-*" />


This PR should simplify that, right? Or do we also want to use the package in apps targeting 8.0?

Or do we also want to use the package in apps targeting 8.0?

Yes. We have no choice, because M.E.AI.Abstractions targets the 9.x versions of these packages (even on net8.0).

We shouldn't be using wildcards *s pacakge versions.

As for using STJ 9 on net8.0, I think that is something to think hard about in MEAI before shipping, since 9 is STS and 8 is LTS.

@SteveMCarroll

@stephentoub The current status for this is that MEAI.Abstractions will depend on STJ v8, whereas MEAI will depend on STJ v9, right?

If so then the Aspire integration package will still have to depend on STJv9 (even on net8.0), because it needs to depend on MEAI (not .Abstractions).

Is this going to be a problem?

@eerhardt Your comment above gave me the impression this was a concern. Can you let me know if it is?

Generally there are 2 concerns with lifting libraries out of the shared framework:

(technical) It is another .dll that needs to be distributed with your application. Also, it is no longer ReadyToRun'd because we aren't using the assembly in the shared framework (which is RID-specific and R2R'd).

(policy) the servicing lifetime of System.Text.Json 9.0 ends before the servicing lifetime of System.Text.Json 8.0. If you are staying on net8.0 TFM, you probably want to keep on a long-term support framework. But using MEAI (not .Abstractions) means that you are signing up for being on the short-term support System.Text.Json. Meaning you will need to move up to the v10 System.Text.Json faster than you would have if you stayed on the v8.

@eerhardt Thanks for clarifying those points. Sorry it's taken a while to get back to this (partly vacation, partly other stuff).

We don't have any proposed plan that I know of for not needing the STJv9 dependency here, since:

It's fundamental to these helpers that they integrate with DI

MEAI's way of doing that requires referencing Microsoft.Extensions.AI since that's where its DI integration lives

Microsoft.Extensions.AI references STJv9

Give this, and the fact that MEAI itself is not going to be in any LTS release prior to .NET 10, we can't provide an LTS-only set of dependencies.

The two concerns you raise certainly both sound valid, but do you see them just as concerns or as blockers in some sense? If you think neither of them actually block us from proceeding then we will do so. But if you think it's not that simple then perhaps we can set up a meeting to discuss.

do you see them just as concerns or as blockers in some sense?

They are concerns, not necessarily blockers. If MEAI decides that it requires STJv9 there isn't anything .NET Aspire can do about it (other than not take the dependency, which I'm not advocating for).

I do think this is an area that needs attention/thought across the whole org. In the past we've had different strategies for dependencies. And our current lifetime/servicing policies make it hard for libraries to depend on STS versions of OOB packages.

Thanks @eerhardt. Based on this I'm going to complete the PR using the API design here.

If anyone doesn't think this is the best API shape then please speak up asap!

SteveSandersonMS · 2024-10-10T21:53:49Z

I think the builder alternative was suggested instead of a lambda in the extension method because we already have two optional lambdas in it (to customize option and settings).

Can you give an example of how that would look?

If you mean something like this, we can certainly look at going that way. However it would prohibit this sort of calling pattern:

builder.AddAzureOpenAIClient("openai")
    .AddChatClient()
    .AddEmbeddingGenerator();

... because if AddChatClient returned something you can chain pipeline builder calls onto, then you wouldn't later be able to call AddEmbeddingGenerator.

SteveSandersonMS · 2024-10-16T07:50:17Z

I'm keen to finish off this PR but to avoid any wasted work can someone from the Aspire side acknowledge if you're happy with the API shape so far? cc @sebastienros @eerhardt @davidfowl

Specifically, this sort of registration usage:

builder.AddAzureOpenAIChatClient("openai");

Or with custom pipeline:

builder.AddAzureOpenAIChatClient("openai", pipeline => pipeline
    .UseChatOptions(...)
    .UseFunctionCalling()); // ... plus any other pipeline builder calls

Alternative

The only other main proposal is the following:

builder.AddAzureOpenAIClient("openai").AddChatClient(pipeline => pipeline
    .UseChatOptions(...)
    .UseFunctionCalling()); // ... plus any other pipeline builder calls

In this case we'd change the return type of AddAzureOpenAIClient/AddOpenAIClient from void to some builder we can put AddChatClient and AddEmbeddingGenerator onto. It has a drawback that you can't call both AddChatClient and AddEmbeddingGenerator without capturing the builder.AddAzureOpenAIClient("openai") result into a separate variable.

davidfowl · 2024-10-16T07:52:29Z

The alternative looks better. EF has a similar problem of 2 lambdas (to configure EF and the underlying provider) and the API does not feel as great.

SteveSandersonMS · 2024-10-16T07:53:44Z

@davidfowl Hold on, I updated the code above, after realising I was coupling together two separate design pivots. Can you re-check the code in that comment?

SteveSandersonMS · 2024-10-16T07:55:24Z

As for the question of whether the IChatClient pipeline should be configured via a lambda or should be chained onto the AddAzureOpenAIChatClient/AddChatClient call, this is a further design pivot covered by https://github.com/dotnet/temp-ai-abstractions/issues/296. Whichever way we go on https://github.com/dotnet/temp-ai-abstractions/issues/296 should result in the same choice on that point here.

In other words, even if that's our end goal we still have to decide whether there's a single AddAzureOpenAIChatClient call, or whether it's a pair (AddAzureOpenAIClient("...").AddChatClient()).

stephentoub · 2024-10-16T12:09:11Z

It has a drawback

We also need to revisit the Use method, which someone might chain at the end (assuming the previous methods return the same ChatClientBuilder) not realizing it's an expensive nop. Renaming it back to Build would help with that.

SteveSandersonMS · 2024-10-16T12:12:30Z

We also need to revisit the Use method, which someone might chain at the end

I agree. We should remove that method completely and make it a constructor parameter to ChatClientBuilder. Once we've settled the API design here, I'll make the API updates across this repo and dotnet/extensions.

sebastienros · 2024-10-16T17:01:36Z

I like both alternatives (with or without builder). But if we expect users to use mostly use chat or embedding clients directly instead of the OpenAIClient then would prefer the non-builder alternative and no lambda.

If we use a builder on AddAzureOpenAIClient then use the lambda instead of having to capture the builder (like we do for withCosmosDBEmulator(p => p.WithVolume())

davidfowl · 2024-10-16T17:45:59Z

If we use a builder on AddAzureOpenAIClient then use the lambda instead of having to capture the builder (like we do for withCosmosDBEmulator(p => p.WithVolume())

Except this isn't an app host API.

eerhardt · 2024-10-16T23:29:16Z

One concern I have is this method:

aspire/src/Components/Aspire.Azure.AI.OpenAI/AspireConfigurableOpenAIExtensions.cs

Lines 21 to 27 in 4f7dbd4

    
           /// <summary> 
        
           /// Registers <see cref="OpenAIClient"/> or <see cref="AzureOpenAIClient"/> as a singleton in the services provided by the <paramref name="builder"/>. 
        
           /// The concrete implementation is selected automatically from configuration. 
        
           /// </summary> 
        
           /// <param name="builder">The <see cref="IHostApplicationBuilder" /> to read config from and add services to.</param> 
        
           /// <param name="connectionName">A name used to retrieve the connection string from the ConnectionStrings configuration section.</param> 
        
           public static void AddOpenAIClientFromConfiguration(

Following the pattern of AddAzureOpenAIChatClient, we would need to duplicate this method for each type of client we wanted: chat, embedding, audio, etc. So we'd have:

AddAzureOpenAIChatClientFromConfiguration
AddAzureOpenAIEmdeddingClientFromConfiguration

Where as if adding the OpenAIClient was separated from adding the "child" chat, embedding, assistant, audio, etc. clients we wouldn't need to duplicate the "from configuration" part.

SteveSandersonMS · 2024-11-26T14:13:15Z

OK, since we got the updated build of Microsoft.Extensions.AI that supports the API patterns needed for the design we want, this PR is now hopefully done.

A principle of .NET Aspire is that telemetry is enabled by default

This PR now aligns with that principle. I've made all the new APIs enable otel for IChatClient and IEmbeddingGenerator<T, U> by default. You can explicitly opt out, e.g.:

builder.AddAzureOpenAIClient("openai").AddChatClient(disableOpenTelemetry: true);

... and then if you want you can call UseOpenTelemetry() manually at some other point in the IChatClient pipeline. By default the open telemetry step is immediately before the inner client in the pipeline, which is almost always what a developer would want.

Have we given any more thought to the Hosting integration APIs? Specifically @sebastienros's proposal at #6577.

I'm totally happy with that and think we should do it. I've already given comments on that PR.

I would like to merge this PR first and then @sebastienros is free to rebase on that. This has already been an extremely long-lived PR so it would be great to close this out.

CC @eerhardt @sebastienros @stephentoub for any final review. If there aren't objections I'll merge this soon since I think we've already been through enough design iterations to believe people are on board with this, and there was nothing surprising that came up in the final implementation.

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIClientBuilder.cs

src/Components/Aspire.OpenAI/AspireOpenAIClientBuilderChatClientExtensions.cs

stephentoub · 2024-11-26T15:47:22Z

src/Components/Aspire.OpenAI/AspireOpenAIClientBuilderChatClientExtensions.cs

+    /// <param name="deploymentName">Optionally specifies which model deployment to use. If not specified, a value will be taken from the connection string.</param>
+    /// <param name="disableOpenTelemetry">Optional. If <see langword="true"/>, skips registering open telemetry support in the <see cref="IChatClient"/> pipeline.</param>
+    /// <returns>A <see cref="ChatClientBuilder"/> that can be used to build a pipeline around the inner <see cref="IChatClient"/>.</returns>
+    public static ChatClientBuilder AddChatClient(


Should this add an OpenAI.ChatClient in addition to an IChatClient?

I tried implementing this (and we can do so) but am not sure it's the right thing to do. Two issues:

Aspire.OpenAI and Aspire.Azure.AI.OpenAI don't have a convention that there should be a OpenAI.ChatClient in DI. Instead they expect there only to be an OpenAIClient. We would be making up a new convention for that, and it's not clear from the API shape that the developer is asking us to register anything other than an IChatClient. We'd be taking a position that the developer does want exactly one OpenAI.ChatClient and it corresponds to the model/deployment you're using for the IChatClient, which is not clear from the API.

It would go wrong if the developer registers multiple unkeyed (or same-keyed) IChatClient instances in DI. The registration for OpenAI.ChatClient would have to be a factory that reads the current IChatClient from DI and then call GetService<OpenAI.ChatClient>() on it (because the OpenAI.ChatClient instance is produced internally by MEAI's OpenAIChatClient). If the developer overwrote the OpenAI-specific IChatClient with a non-OpenAI IChatClient in their DI registry, that lookup would fail. That is, they would have a service collection entry for the OpenAI.ChatClient type, but resolving it would fail for what seems like an unrelated reason.

Altogether, given that it's not an existing convention to register a singleton OpenAI.ChatClient in DI, and since any developer who wants to get the OpenAI.ChatClient corresponding to their IChatClient easily can do so [1], I would suggest we don't add a further behavior to document and maintain here. At least not now - it's easier to add this in the future than it would be to remove it.

[1] Either by calling chatClient.GetService<OpenAI.ChatClient>() at the usage site, or by registering it as a singleton with builder.Services.AddSingleton(s => s.GetService<IChatClient>().GetService<OpenAI.ChatClient>()).

src/Components/Aspire.OpenAI/AspireOpenAIClientBuilderChatClientExtensions.cs

src/Components/Aspire.OpenAI/AspireOpenAIClientBuilderEmbeddingGeneratorExtensions.cs

src/Components/Aspire.OpenAI/AspireOpenAIClientBuilderChatClientExtensions.cs

playground/OpenAIEndToEnd/OpenAIEndToEnd.WebStory/Components/Pages/UseIChatClient.razor

…ing OpenAI E2E test setup

SteveSandersonMS · 2024-11-27T15:43:47Z

I believe this addresses all the remaining feedback (except possibly this). We've been through enough design iterations to be sure everyone's on board, and there's an approval already. Since most people will now be out for most of a week, I'll merge now rather than hanging around potentially for a long time for further approvals.

If anyone has further comments please let me know!

eerhardt · 2024-11-27T16:58:47Z

Thanks for the great contribution here, @SteveSandersonMS. I think we are in a much better place w.r.t. supporting OpenAI in .NET Aspire, and I'm excited to see what people build with this functionality.

SteveSandersonMS requested a review from stephentoub October 10, 2024 08:43

SteveSandersonMS requested review from radical, eerhardt and sebastienros as code owners October 10, 2024 08:43

SteveSandersonMS mentioned this pull request Oct 10, 2024

Review all IChatClient implementations for thread-safety and suitability as singleton dotnet/extensions#5499

Closed

stephentoub closed this Oct 10, 2024

stephentoub reopened this Oct 10, 2024

stephentoub reviewed Oct 10, 2024

View reviewed changes

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 10, 2024

View reviewed changes

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 10, 2024

View reviewed changes

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs Outdated Show resolved Hide resolved

stephentoub approved these changes Oct 10, 2024

View reviewed changes

sebastienros reviewed Oct 10, 2024

View reviewed changes

SteveSandersonMS added 8 commits November 26, 2024 11:25

Same for embedding generator

27e40e5

Open telemetry by default

d280001

Tests for configuring middleware

5067628

Fix namespace

20bcdcd

Add AspireOpenAIClientBuilder

0864053

Add OpenAI variants

a3c4694

OpenAI unit tests

71cf2d6

Avoid duplicated logic

c2d2580

SteveSandersonMS commented Nov 26, 2024

View reviewed changes

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIClientBuilder.cs Show resolved Hide resolved

Remove incorrect remarks

4e07616

stephentoub reviewed Nov 26, 2024

View reviewed changes

src/Components/Aspire.OpenAI/AspireOpenAIClientBuilderChatClientExtensions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 26, 2024

View reviewed changes

stephentoub approved these changes Nov 26, 2024

View reviewed changes

This was referenced Nov 26, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

Some tests timing out on helix #5247

Open

eerhardt reviewed Nov 26, 2024

View reviewed changes

sebastienros reviewed Nov 26, 2024

View reviewed changes

src/Components/Aspire.OpenAI/AspireOpenAIClientBuilderChatClientExtensions.cs Outdated Show resolved Hide resolved

eerhardt reviewed Nov 26, 2024

View reviewed changes

playground/OpenAIEndToEnd/OpenAIEndToEnd.WebStory/Components/Pages/UseIChatClient.razor Show resolved Hide resolved

SteveSandersonMS added 6 commits November 27, 2024 13:38

Factor out some shared logic

61b1ddb

Add design comment

b3e997a

In E2E test, pass deployment name manually for consistency with exist…

0237c9b

…ing OpenAI E2E test setup

Expose "DisableTracing" flag on AspireOpenAIClientBuilder

4c29078

Test tidyup

225cfec

Control tracing/telemetry using higher-level OpenAI/AzureOpenAI config

6faa8e2

SteveSandersonMS merged commit 04cf1fe into main Nov 27, 2024
9 checks passed

SteveSandersonMS deleted the stevesa/meai-integration branch November 27, 2024 15:44

github-actions bot locked and limited conversation to collaborators Dec 28, 2024

Add AddAzureOpenAIChatClient #6225

Add AddAzureOpenAIChatClient #6225

Conversation

SteveSandersonMS commented Oct 10, 2024 • edited Loading

Description

Design questions

Lifetime

APIs for chaining on AddAzureOpenAIClient

Checklist

Microsoft Reviewers: Open in CodeFlow

SteveSandersonMS commented Oct 10, 2024

stephentoub commented Oct 10, 2024

stephentoub commented Oct 10, 2024 • edited Loading

sebastienros commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Oct 10, 2024

stephentoub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Oct 10, 2024

SteveSandersonMS commented Oct 16, 2024 • edited Loading

Alternative

davidfowl commented Oct 16, 2024 • edited Loading

SteveSandersonMS commented Oct 16, 2024 • edited Loading

SteveSandersonMS commented Oct 16, 2024 • edited Loading

stephentoub commented Oct 16, 2024 • edited Loading

SteveSandersonMS commented Oct 16, 2024 • edited Loading

sebastienros commented Oct 16, 2024

davidfowl commented Oct 16, 2024

eerhardt commented Oct 16, 2024

SteveSandersonMS commented Nov 26, 2024

Choose a reason for hiding this comment

SteveSandersonMS Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

SteveSandersonMS commented Nov 27, 2024

eerhardt commented Nov 27, 2024

SteveSandersonMS commented Oct 10, 2024 •

edited

Loading

APIs for chaining on `AddAzureOpenAIClient`

stephentoub commented Oct 10, 2024 •

edited

Loading

stephentoub commented Oct 10, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

davidfowl commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

stephentoub commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS Nov 27, 2024 •

edited

Loading