Create diff for generator for multipart/form-data by annelo-msft · Pull Request #1 · annelo-msft/openai-in-typespec

annelo-msft · 2024-03-15T02:31:08Z

No description provided.

…ai-generate-multipart

annelo-msft · 2024-03-15T04:54:52Z

-            using RequestBody content = audio.ToRequestBody();
-            Result result = await CreateTranscriptionAsync(content, context).ConfigureAwait(false);
-            return Result.FromValue(CreateTranscriptionResponse.FromResponse(result.GetRawResponse()), result.GetRawResponse());
+            (BinaryContent content, string contentType, RequestOptions options) = await audio.ToMultipartContentAsync().ConfigureAwait(false);


This is where we obtain the multipart content instead of standard BinaryContent.

This part looks different in #2 and #3.

Azure/autorest.csharp#4340

annelo-msft · 2024-03-18T20:44:20Z

        /// <param name="model"> ID of the model to use. Only `whisper-1` is currently available. </param>
        /// <exception cref="ArgumentNullException"> <paramref name="file"/> is null. </exception>
-        public CreateTranscriptionRequest(BinaryData file, CreateTranscriptionRequestModel model)
+        public CreateTranscriptionRequest(Stream file, string fileName, CreateTranscriptionRequestModel model)


For MPFD, if there is a file-upload parameter/model property, we need the binary-holding type to be a Stream and an option for the end-user to pass a string with the filename (they can also pass null, but nullable is disabled in this file, so it's only visible here by the fact that the method doesn't throw ArgumentException if null is passed for it). So, we would generate two properties from whatever the TypeSpec is that indicates you can upload a file using the service endpoint.

Azure/autorest.csharp#4488

…ai-generate-multipart

annelo-msft · 2024-03-26T19:04:04Z

        /// <param name="speech"> The <see cref="CreateSpeechRequest"/> to use. </param>
-        /// <param name="cancellationToken"> The cancellation token to use. </param>
        /// <exception cref="ArgumentNullException"> <paramref name="speech"/> is null. </exception>
-        public virtual async Task<ClientResult<BinaryData>> CreateSpeechAsync(CreateSpeechRequest speech, CancellationToken cancellationToken = default)


Unbranded clients don't take cancellation tokens in convenience methods.

Azure/autorest.csharp#4243

annelo-msft · 2024-03-26T19:04:25Z

        /// <list type="bullet">
        /// <item>
        /// <description>
-        /// This <see href="https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/samples/ProtocolMethods.md">protocol method</see> allows explicit creation of the request and processing of the response for advanced scenarios.


Maybe we want to remove Azure in the docs here?

Azure/autorest.csharp#4481

annelo-msft · 2024-03-26T19:07:11Z

        /// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
        /// <returns> The response returned from the service. </returns>
-        public virtual async Task<ClientResult> CreateTranscriptionAsync(BinaryContent content, RequestOptions context = null)
+        public virtual async Task<ClientResult> CreateTranscriptionAsync(BinaryContent content, string contentType, RequestOptions options = null)


MPFD protocol methods take contentType -- this is the value of the Content-Type header. This allows the same protocol method to be generated in cases where the service endpoint can accept multiple different content types -- the caller can specify, and we can provide convenience methods for the version that works best for the client.

Azure/autorest.csharp#4340

annelo-msft · 2024-03-26T19:08:53Z

            request.Headers.Set("Accept", "application/octet-stream");
            request.Headers.Set("Content-Type", "application/json");
            request.Content = content;
+            message.Apply(options);


The call to message.Apply(options) needs to move to the end of the request-creation helper method so that the semantics of SetHeader on RequestOptions are correct. This is because anything the user called SetHeader with should replace what the generated code does.

Azure/autorest.csharp#4484

annelo-msft · 2024-03-26T20:54:26Z

        {
            await pipeline.SendAsync(message).ConfigureAwait(false);

-            if (message.Response == null)


We don't need to validate this in unbranded clients.

Azure/autorest.csharp#4485

annelo-msft · 2024-03-26T20:55:07Z

            await pipeline.SendAsync(message).ConfigureAwait(false);

-            if (message.Response == null)
+            if (message.Response!.IsError && options.ErrorOptions == ClientErrorBehaviors.Default)


We should clarify the positive case where we throw, instead of the negative case. The method should end returning a successful result to clarify what's happening in it.

Azure/autorest.csharp#4485

annelo-msft · 2024-03-26T20:55:22Z

 {
    internal static class ClientPipelineExtensions
    {
-        public static async ValueTask<PipelineResponse> ProcessMessageAsync(this ClientPipeline pipeline, PipelineMessage message, RequestOptions requestContext, CancellationToken cancellationToken = default)


We don't use the cancellationToken and shouldn't give the option to pass it.

annelo-msft · 2024-03-26T20:55:47Z

+            if (message.Response!.IsError && options.ErrorOptions == ClientErrorBehaviors.Default)
            {
-                throw new InvalidOperationException("Failed to receive Result.");
+                throw await ClientResultException.CreateAsync(message.Response).ConfigureAwait(false);


In an async context, we need to call the factory method to create the exception asynchronously.

Azure/autorest.csharp#4485

annelo-msft · 2024-03-26T20:57:36Z

+            return message.Response;
        }

        public static async ValueTask<ClientResult<bool>> ProcessHeadAsBoolMessageAsync(this ClientPipeline pipeline, PipelineMessage message, RequestOptions requestContext)


I don't think this is the correct logic for unbranded clients. We don't have to revisit or block on this today for OpenAI, but we should address it soon so we don't ship an unbranded client with the wrong behavior.

One specific problem here is that we don't honor any response classifiers an end-user might pass. There may be others as well, e.g. I don't think we want to return exploding responses from unbranded client -- it is not idiomatic to .NET to throw from properties on types.

Tracking with Azure/autorest.csharp#4479

annelo-msft · 2024-03-26T21:02:51Z

        /// <exception cref="ArgumentNullException"> <paramref name="content"/> is null. </exception>
        /// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
        /// <returns> The response returned from the service. </returns>
-        public virtual async Task<ClientResult> CreateSpeechAsync(BinaryContent content, RequestOptions context = null)


Since the variable name is public, we need to change context to options.

Azure/autorest.csharp#4482

annelo-msft · 2024-03-26T21:03:58Z

        }

-        internal PipelineMessage CreateCreateSpeechRequest(BinaryContent content, RequestOptions context)
+        private PipelineMessage CreateCreateSpeechRequest(BinaryContent content, RequestOptions options)


It is better for these to be private if there isn't a clear need for them to be internal.

Azure/autorest.csharp#4483

I do not think we need this to be "internal", because other parts of our generated libraries are not using them.
But I prefer we change this later after all the changes that affect functionalities are resolved.

@ArcturusZhang, that's fine, I agree this is lower priority than addressing public API or functional issues. Thanks!

annelo-msft · 2024-03-26T21:04:36Z

 // <auto-generated/>

-#nullable disable
+#nullable enable


This is important for a "ClientModel" type so we can do proper validation on nulls.

well the generated code will always have "nullable disable", therefore we cannot do this unless we change all of them in generated code (including models clients) and all other dependencies (such as resourcemanager)

@ArcturusZhang, why is generated code not able to change #nullable disable to #nullable enable for select emitted files?

We can change. But I believe we really do not have to change it now.
The generator does not support generating nullable context yet.

@ArcturusZhang: I think the question that needs to be answered here is why are we looking at this as "all or nothing?" I would think that it should be possible to emit nullable in the scope of the unbranded writer without impacting DPG/MGP. Is that not the case, and if not - why?

@jsquire Yes it is possible to support that but not now, we could put this as a future plan.
I was trying to explain why we cannot do that now, and as a result, my PRs will not change the nullable context
cc @m-nash for further clarification if needed

annelo-msft · 2024-03-26T21:10:23Z

+// <auto-generated/>

-#nullable disable
+#nullable enable


I don't have strong opinions about this implementation -- I just copied over the one from the customized OpenAI.

Tracking bug fixes for this with this issue: Azure/autorest.csharp#4480

annelo-msft · 2024-03-26T21:19:13Z


        /// <summary> Deserializes the model from a raw response. </summary>
-        /// <param name="response"> The result to deserialize the model from. </param>
+        /// <param name="response"> The response to deserialize the model from. </param>


These changes are low-priority since the APIs are internal, but if they are easy, they would make the code more readable and correct.

Azure/autorest.csharp#4486

annelo-msft · 2024-03-26T21:20:32Z


 namespace OpenAI.Models
 {
-    public partial class CreateTranscriptionRequest : IJsonModel<CreateTranscriptionRequest>


I don't know offhand if we want MPFD input models to support IPersistableModel, but they are not JSON, so probably not IJsonModel. Let me know if you want to discuss what IPersistableModel would look like and on what timeline we would want to solve that.

Azure/autorest.csharp#4487

chunyu3 · 2024-04-02T01:59:34Z

        /// </para>
        /// </summary>
-        public BinaryData File { get; }
+        public Stream File { get; }


Split the information of a file to upload in Model as separate Properties works on the scenario which only have one file to upload, but it does not work on multiple files to upload. (Azure/autorest.csharp#4488 (comment) it add additional Properties to the model which may break the model. Considering the model will be used in both multipart and application/json, it may not correct to serialize to json format.
How about we define an new type for File in Multipart, as Azure/autorest.csharp#4488 (comment)

Responded to this comment here: #3 (comment)

ArcturusZhang · 2024-04-02T07:41:26Z

Changes in this file is processing in this PR: Azure/autorest.csharp#4516

ArcturusZhang · 2024-04-02T07:42:40Z

            {
                DateTimeKind.Utc => ToString((DateTimeOffset)value, format),
-                _ => throw new NotSupportedException($"DateTime {value} has a Kind of {value.Kind}. Azure SDK requires it to be UTC. You can call DateTime.SpecifyKind to change Kind property value to DateTimeKind.Utc.")
+                _ => throw new NotSupportedException($"DateTime {value} has a Kind of {value.Kind}. Generated clients require it to be UTC. You can call DateTime.SpecifyKind to change Kind property value to DateTimeKind.Utc.")


This change is resolved in this PR: Azure/autorest.csharp#4516

ArcturusZhang · 2024-04-02T08:19:53Z

+        PathBuilder.Append(value);
+        UriBuilder.Path = PathBuilder.ToString();


comparing with the old code, we removed the pathSeparator part in this method. This is included in this PR: Azure/autorest.csharp#4519

I did not include other removals in this PR yet. Will those break the current generated code? Those are various of overloads of the two remaining methods.
Would those removals require generator code changes in the clients? (I assume so)
If so, I suggest we delay those parts of changes, and prioritize others.

I'm not sure I fully understand your question or the concern here -- but please clarify, I am happy to answer any questions on this. I had filed Azure/autorest.csharp#4480 to track updates to ClientUriBuilder -- we have some problems with the current implementation around escaping that will need to be resolved, but if the OpenAI generated client is functional, we can de-prioritize those for the time being.

Let me rephrase myself: I compared the new code and old code of this file, because here github is not properly comparing it.
Your change consists of two parts:

The first part included in this change: Azure/autorest.csharp@e5e175f (#4519)

The second part is that we are removing those variant of AppendPath and AppendQuery (for instance the overload that takes DateTimeOffset). I am wondering that would this causes compile errors in the generated clients? Are you removing those because your specific case is not using those methods, or removing them is part of the issue?

In the meantime, could you have a review on this PR? Now this PR is very simple, I only included the first part of the change, which resolves an issue reported by openai

ArcturusZhang · 2024-04-03T03:42:13Z

Sign in to view

        /// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
        /// <returns> The response returned from the service. </returns>
-        public virtual async Task<ClientResult> CreateTranslationAsync(BinaryContent content, RequestOptions context = null)
+        public virtual async Task<ClientResult> CreateTranslationAsync(BinaryContent content, string contentType, RequestOptions options = null)


OK I saw the other comments that this change belongs to MPFD changes

ArcturusZhang · 2024-04-03T03:30:00Z

-        /// <param name="cancellationToken"> The cancellation token to use. </param>
        /// <exception cref="ArgumentNullException"> <paramref name="audio"/> is null. </exception>
-        public virtual ClientResult<CreateTranslationResponse> CreateTranslation(CreateTranslationRequest audio, CancellationToken cancellationToken = default)
+        public virtual ClientResult<CreateTranslationResponse> CreateTranslation(CreateTranslationRequest audio)


this change has been included in this PR: Azure/autorest.csharp#4524

ArcturusZhang · 2024-04-03T03:30:33Z

+            (BinaryContent content, string contentType, RequestOptions options) = audio.ToMultipartContentAsync().GetAwaiter().GetResult();
+            ClientResult result = CreateTranslation(content, contentType, options);


I believe this is part of the multipart change, this PR does not contain this change.

ArcturusZhang · 2024-04-03T03:40:54Z

        /// <item>
        /// <description>
-        /// This <see href="https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/samples/ProtocolMethods.md">protocol method</see> allows explicit creation of the request and processing of the response for advanced scenarios.
+        /// This <see href="https://azsdk/net/protocol/quickstart">protocol method</see> allows explicit creation of the request and processing of the response for advanced scenarios.


is this link correct? I cannot open it.

I uses Michael's alternative: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/samples/ServiceMethods.md#protocol-methods
from here: Azure/autorest.csharp#4481 (comment)

It should be https://aka.ms/azsdk/net/protocol-methods

Sure I will change the link to this new one

annelo-msft added 6 commits March 14, 2024 19:29

upgrade to v1.1.0-beta.3

6f042b8

remove tests

9439b9d

Merge remote-tracking branch 'origin/openai-generate-clean' into open…

32df56e

…ai-generate-multipart

request helper whitespace

b048a57

Merge branch 'openai-generate-clean' into openai-generate-multipart

d9f2e3b

updates for multipart

db1b736