Investigation: Azure and unbranded clients by annelo-msft · Pull Request #5 · annelo-msft/openai-in-typespec

annelo-msft · 2024-04-08T21:10:38Z

This PR looks at the model-inheritance approach.

annelo-msft · 2024-04-09T23:19:51Z

+using OpenAI.Models;
+using System.ClientModel;
+
+using AzureOpenAI.Models;


Once you add the namespace with the extension methods/properties, you can use an unbranded model like an Azure model. How do we make sure people instantiate the client as an Azure client and only send Azure properties to the Azure service?

annelo-msft · 2024-04-09T23:28:40Z

+
+namespace AzureOpenAI;
+
+internal class PersistableModelList<TModel> : List<IPersistableModel<TModel>>, IPersistableModel<PersistableModelList<TModel>>


This is a proposal for a new emitted type.

annelo-msft · 2024-04-09T23:29:30Z

+[PersistableModelProxy(typeof(UnknownAzureChatExtensionConfiguration))]
+public partial class AzureChatExtensionConfiguration : IJsonModel<AzureChatExtensionConfiguration>
+{
+    void IJsonModel<AzureChatExtensionConfiguration>.Write(Utf8JsonWriter writer, ModelReaderWriterOptions options)


Per @KrzysztofCwalina, look at different options for "Wire" and "AzureWire"

We decided to move away from this approach in favor of having separate models for each type.

annelo-msft · 2024-04-09T23:31:28Z

+
+namespace AzureOpenAI.Models;
+
+internal class AzureCreateChatCompletionRequest : CreateChatCompletionRequest


I think this goes away entirely.

Actually, maybe this is better than keeping a Dictionary<string, object> ...

Discussed offline and for now we will continue to pursue the extension methods approach

New approach is to keep these model subtypes types internal and use extension properties to retrieve values via internal properties on subtypes.

annelo-msft · 2024-04-09T23:34:25Z

+
+namespace AzureOpenAI.Models;
+
+public partial class AzureSearchChatExtensionConfiguration :  IJsonModel<AzureSearchChatExtensionConfiguration>


Note, some models like these are only in the Azure package. But we assign instances of them at runtime to objects in the unbranded package and reference them as IJsonModel. Think a bit more about this and the implications of it.

One angle: what does the generator do?
Side question on that - how does the Tsp represent the differences so the generator can do a different thing?

annelo-msft · 2024-04-09T23:37:16Z

        /// <param name="keyCredential"> The key credential to copy. </param>
        /// <param name="endpoint"> OpenAI Endpoint. </param>
-        internal Chat(ClientPipeline pipeline, ApiKeyCredential keyCredential, Uri endpoint)
+        protected internal Chat(ClientPipeline pipeline, ApiKeyCredential keyCredential, Uri endpoint)


The subclient constructor becomes protected internal.

annelo-msft · 2024-04-09T23:37:37Z

        private readonly ClientPipeline _pipeline;
        private readonly Uri _endpoint;

+        protected Uri Endpoint => _endpoint;


Some client internal fields become protected properties so the derived type can use them.

annelo-msft · 2024-04-09T23:38:12Z

        }

-        internal PipelineMessage CreateCreateChatCompletionRequest(BinaryContent content, RequestOptions context)
+        protected virtual PipelineMessage CreateCreateChatCompletionRequest(BinaryContent content, RequestOptions context)


Request creation helpers become protected virtual.

annelo-msft · 2024-04-09T23:38:46Z

+
+            // Note: we were holding strongly-typed values for added Azure properties
+            // We have to serialize them separately here.
+            // TODO: Does the format matter here?  It seems like we want to write them


Look at this in the context of different options for Wire-3rd party and Wire-Azure.

annelo-msft · 2024-04-09T23:41:54Z

+
+                    writer.WritePropertyName(property.Key);
+
+                    // Note: what if it's just a primitive, i.e. a string or int, what do we do?


TODO to solve.

annelo-msft · 2024-04-09T23:42:10Z

        /// </summary>
        private IDictionary<string, BinaryData> _serializedAdditionalRawData;

+        private IDictionary<string, object> _additionalTypedProperties;


TODO to solve.

annelo-msft · 2024-04-09T23:43:05Z

    public partial class OpenAIClient
    {
+        protected ApiKeyCredential KeyCredential => _keyCredential;
+        protected Uri Endpoint => _endpoint;


Some client fields become protected properties so that they're accessible to derived clients. Consider this approach.

annelo-msft · 2024-04-09T23:43:35Z

    /// <summary> The OpenAI service client. </summary>
    public partial class OpenAIClient
    {
+        protected ApiKeyCredential KeyCredential => _keyCredential;


One problem with this is that if the client doesn't use api key auth, what does this do?

annelo-msft · 2024-04-09T23:44:29Z

+    {
+    }
+
+    // TODO: Show how this would differ for this case.  Do we still need OperationName and the


Confirmed we don't need OperationName for this approach.

annelo-msft · 2024-04-11T21:43:34Z

+        // and stick it in AdditionalTypedProperties (are these "deserialized
+        // additional properties, e.g.?).
+
+        // TODO: what is our concurrency story for models in unbranded clients?


https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-service-models-namespace

annelo-msft · 2024-04-11T21:44:58Z

+{
+    public static IList<AzureChatExtensionConfiguration> GetDataSources(this CreateChatCompletionRequest request)
+    {
+        // TODO: How can we validate that this is being called in the right context,


How can we throw an exception when using the wrong client? Seems like MRW.Options is useful here.

annelo-msft · 2024-04-11T21:47:34Z

+        //   2. The extension properties are actually quite simple -- they just get
+        //      or set stuff in either the input or output properties dictionaries.
+        //
+        // TODO: What is the interplay of SerializedAdditionalRawData and 


…col method to Azure client

KrzysztofCwalina · 2024-04-12T22:01:00Z

+        Argument.AssertNotNull(createChatCompletionRequest, nameof(createChatCompletionRequest));
+
+        // Initialize Azure input model
+        AzureCreateChatCompletionRequest azureChatCompletionOptions = new(createChatCompletionRequest);


Could you just wrap and then call the base implementation? It would be good to not have to reimplement the convenience methods totally from scratch.

I think when the protocol method signature is the same that will work. But since here we needed to add an overload for the protocol method because an input value has moved from the request body into a path parameter, we need to reimplement it to call the new protocol method overload.

Ah, makes sense. But, where is the new protocol method? I have a hard time finding it. I was interested in seeing how you think we will make the new protocol method available to the callers.

OK, I found the protocol method. But, the method needs to be an extension method, not just a public method on the internal type. People won't be able to call it, if it's an instance method

Good point! That is my mistake.

KrzysztofCwalina · 2024-04-12T22:02:59Z

+    string IPersistableModel<AzureCreateChatCompletionRequest>.GetFormatFromOptions(ModelReaderWriterOptions options)
+        => "J";
+
+    void IJsonModel<AzureCreateChatCompletionRequest>.Write(Utf8JsonWriter writer, ModelReaderWriterOptions options)


likewise, I wonder if we could reuse the serialization implementation for the things that are the same and only add/modify the parts that are different.

That seems likely. My question on that would be -- what is the value of doing it? I'm still wondering how we represent the added properties in the TypeSpec to indicate to the generator to generate just the diff from the base type. Once we solve that piece , it'll be easier to say which approach would be less work for the generator. If there's strong value to not duplicating generated serialization code, then we can prioritize figuring that part out.

yeah, it's a good point that it will be easier on the generator to not have to understand such tricks. As far as the generator will be concerned is that there are two model types and two typespecs. It will generate serialization code as if there was no hierarchy,

KrzysztofCwalina · 2024-04-12T22:52:09Z


    public override Task<ClientResult> CreateChatCompletionAsync(BinaryContent content, RequestOptions context = null)
    {
+        // Note, that we can later remap the values from the 3rd party client format to the 


it would be good to hide this one, but I don't think we can do it given we don't expose the subtype

Yes, that makes sense. If we did expose the subtype, though, I think is a clear violation of the Liskov substitution principle -- I think you told me a few weeks ago that if we hide base type methods on the subtype, it makes the abstraction confusing ... why could I call this method on the base client, and now I don't see that method in the intellisense when I'm looking at a subtype? I dunno.

…ses 1-1 with the type they extend

annelo-msft added 7 commits April 8, 2024 14:09

Add Azure client

df95c53

updates

60575d9

updates

45b680a

beginning work on extension methods

51e2eec

WIP

0ac5efe

Initial implementation of settable extension property

8ca386f

functional prototype for inputs; with lots of open questions

443c709

annelo-msft commented Apr 9, 2024

View reviewed changes

Comment thread tsp-output/@azure-tools/typespec-csharp/azoai/AzureOpenAI/Models/AzureModelExtensions.cs Outdated

annelo-msft commented Apr 9, 2024

View reviewed changes

annelo-msft added 6 commits April 10, 2024 13:19

updates

43b4afe

add extension and models for output

5df5287

initial try for output models

e29dafa

reorganize demo app code

9c4abdf

Rework IList input property to align with current model design

ea107b2

more notes

fd48c25

annelo-msft commented Apr 11, 2024

View reviewed changes

annelo-msft added 10 commits April 11, 2024 15:23

add in mocks for transport

2503ad1

update mocks; works for unbranded

3c014f0

Make Azure one work too

136c015

rewrite URI and dump content to console

66b0264

rewrite request content dump

50caabe

start work on writing models differently via MRW Options

050f33e

Add exception to throw if extension property called in wrong context

4481c19

add notes to extension methods

2ac6464

make request creation methods internal again -- add overload of proto…

063f1f3

…col method to Azure client

Illustrate input model serialization proposal

7d0d717

KrzysztofCwalina reviewed Apr 12, 2024

View reviewed changes

Illustrate output model extension properties proposal

ca3d8d5

KrzysztofCwalina reviewed Apr 12, 2024

View reviewed changes

annelo-msft added 2 commits April 12, 2024 16:55

Add extension methods to expose protocol method overloads

64f1ee2

reorganize into input and output models and move extensions into clas…

4ee8e8d

…ses 1-1 with the type they extend


		namespace AzureOpenAI;

		internal class PersistableModelList<TModel> : List<IPersistableModel<TModel>>, IPersistableModel<PersistableModelList<TModel>>


		namespace AzureOpenAI.Models;

		internal class AzureCreateChatCompletionRequest : CreateChatCompletionRequest


		namespace AzureOpenAI.Models;

		public partial class AzureSearchChatExtensionConfiguration : IJsonModel<AzureSearchChatExtensionConfiguration>


		writer.WritePropertyName(property.Key);

		// Note: what if it's just a primitive, i.e. a string or int, what do we do?

Conversation

annelo-msft commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annelo-msft Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

annelo-msft commented Apr 8, 2024 •

edited

Loading

annelo-msft Apr 9, 2024 •

edited

Loading