Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6f042b8
upgrade to v1.1.0-beta.3
annelo-msft Mar 15, 2024
9439b9d
remove tests
annelo-msft Mar 15, 2024
32df56e
Merge remote-tracking branch 'origin/openai-generate-clean' into open…
annelo-msft Mar 15, 2024
b048a57
request helper whitespace
annelo-msft Mar 15, 2024
d9f2e3b
Merge branch 'openai-generate-clean' into openai-generate-multipart
annelo-msft Mar 15, 2024
db1b736
updates for multipart
annelo-msft Mar 15, 2024
4eeb4e0
nits
annelo-msft Mar 15, 2024
04f2bfb
nits
annelo-msft Mar 15, 2024
65a4534
updates
annelo-msft Mar 15, 2024
de3877b
updates
annelo-msft Mar 15, 2024
0864377
add boundary; add filename
annelo-msft Mar 18, 2024
7fa6051
nits
annelo-msft Mar 18, 2024
8a755f7
update sync-over-async calls
annelo-msft Mar 18, 2024
00b0bb9
updates
annelo-msft Mar 18, 2024
37100c3
Merge remote-tracking branch 'origin/openai-generate-clean' into open…
annelo-msft Mar 26, 2024
b4b93b8
updates
annelo-msft Mar 26, 2024
6abcc6f
updates
annelo-msft Mar 26, 2024
50b5464
updates
annelo-msft Mar 26, 2024
9dc5ba7
updates
annelo-msft Mar 26, 2024
b8510b7
updates
annelo-msft Mar 26, 2024
40654dc
Merge branch 'openai-generate-clean' into openai-generate-multipart
annelo-msft Mar 26, 2024
c62c465
Merge branch 'openai-generate-clean' into openai-generate-multipart
annelo-msft Mar 26, 2024
1591045
updates
annelo-msft Mar 26, 2024
e5c4472
Merge branch 'openai-generate-clean' into openai-generate-multipart
annelo-msft Mar 26, 2024
a87b943
updates
annelo-msft Mar 26, 2024
61ac653
updates
annelo-msft Mar 26, 2024
15fe4f8
updates
annelo-msft Mar 26, 2024
430e528
updates
annelo-msft Mar 26, 2024
f110e57
updates
annelo-msft Mar 26, 2024
9a369c3
updates
annelo-msft Mar 26, 2024
ab41b3e
updates
annelo-msft Mar 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 79 additions & 96 deletions tsp-output/@azure-tools/typespec-csharp/src/Generated/Audio.cs

Large diffs are not rendered by default.

Copy link
Copy Markdown

@ArcturusZhang ArcturusZhang Apr 2, 2024

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -1,49 +1,37 @@
// <auto-generated/>

#nullable disable
#nullable enable
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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?

Copy link
Copy Markdown

@ArcturusZhang ArcturusZhang Apr 3, 2024

Choose a reason for hiding this comment

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

@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


using System;
using System.ClientModel;
using System.ClientModel.Primitives;
using System.Threading;
using System.Threading.Tasks;

namespace OpenAI
{
internal static class ClientPipelineExtensions
{
public static async ValueTask<PipelineResponse> ProcessMessageAsync(this ClientPipeline pipeline, PipelineMessage message, RequestOptions requestContext, CancellationToken cancellationToken = default)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

public static async ValueTask<PipelineResponse> ProcessMessageAsync(this ClientPipeline pipeline, PipelineMessage message, RequestOptions options)
{
await pipeline.SendAsync(message).ConfigureAwait(false);

if (message.Response == null)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

if (message.Response!.IsError && options.ErrorOptions == ClientErrorBehaviors.Default)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

{
throw new InvalidOperationException("Failed to receive Result.");
throw await ClientResultException.CreateAsync(message.Response).ConfigureAwait(false);
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

}

if (!message.Response.IsError || requestContext?.ErrorOptions == ClientErrorBehaviors.NoThrow)
{
return message.Response;
}

throw new ClientResultException(message.Response);
return message.Response;
}

public static PipelineResponse ProcessMessage(this ClientPipeline pipeline, PipelineMessage message, RequestOptions requestContext, CancellationToken cancellationToken = default)
public static PipelineResponse ProcessMessage(this ClientPipeline pipeline, PipelineMessage message, RequestOptions options)
{
pipeline.Send(message);

if (message.Response == null)
{
throw new InvalidOperationException("Failed to receive Result.");
}

if (!message.Response.IsError || requestContext?.ErrorOptions == ClientErrorBehaviors.NoThrow)
if (message.Response!.IsError && options.ErrorOptions == ClientErrorBehaviors.Default)
{
return message.Response;
throw new ClientResultException(message.Response);
}

throw new ClientResultException(message.Response);
return message.Response;
}

public static async ValueTask<ClientResult<bool>> ProcessHeadAsBoolMessageAsync(this ClientPipeline pipeline, PipelineMessage message, RequestOptions requestContext)
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Expand Down
Original file line number Diff line number Diff line change
@@ -1,210 +1,82 @@
// <auto-generated/>
// <auto-generated/>

#nullable disable
#nullable enable
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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


using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace OpenAI
{
internal class ClientUriBuilder
{
private UriBuilder _uriBuilder;
private StringBuilder _pathBuilder;
private StringBuilder _queryBuilder;
private const char PathSeparator = '/';

public ClientUriBuilder()
{
}

private UriBuilder UriBuilder => _uriBuilder ??= new UriBuilder();

private StringBuilder PathBuilder => _pathBuilder ??= new StringBuilder(UriBuilder.Path);

private StringBuilder QueryBuilder => _queryBuilder ??= new StringBuilder(UriBuilder.Query);

public void Reset(Uri uri)
{
_uriBuilder = new UriBuilder(uri);
_pathBuilder = new StringBuilder(UriBuilder.Path);
_queryBuilder = new StringBuilder(UriBuilder.Query);
}

public void AppendPath(string value, bool escape)
{
Argument.AssertNotNullOrWhiteSpace(value, nameof(value));

if (escape)
{
value = Uri.EscapeDataString(value);
}

if (value[0] == PathSeparator)
{
value = value.Substring(1);
}

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

public void AppendPath(bool value, bool escape = false)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}

public void AppendPath(float value, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}

public void AppendPath(double value, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}

public void AppendPath(int value, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}

public void AppendPath(byte[] value, string format, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value, format), escape);
}

public void AppendPath(IEnumerable<string> value, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}

public void AppendPath(DateTimeOffset value, string format, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value, format), escape);
}

public void AppendPath(TimeSpan value, string format, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value, format), escape);
}

public void AppendPath(Guid value, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}
namespace OpenAI;

public void AppendPath(long value, bool escape = true)
{
AppendPath(ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}

public void AppendQuery(string name, string value, bool escape)
{
Argument.AssertNotNullOrWhiteSpace(name, nameof(name));
Argument.AssertNotNullOrWhiteSpace(value, nameof(value));

if (QueryBuilder.Length == 0)
{
QueryBuilder.Append('?');
}
else
{
QueryBuilder.Append('&');
}

if (escape)
{
value = Uri.EscapeDataString(value);
}

QueryBuilder.Append(name);
QueryBuilder.Append('=');
QueryBuilder.Append(value);
}
internal class ClientUriBuilder
{
private UriBuilder? _uriBuilder;
private StringBuilder? _pathBuilder;
private StringBuilder? _queryBuilder;

public void AppendQuery(string name, bool value, bool escape = false)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}
private UriBuilder UriBuilder => _uriBuilder ??= new();
private StringBuilder PathBuilder => _pathBuilder ??= new(UriBuilder.Path);
private StringBuilder QueryBuilder => _queryBuilder ??= new(UriBuilder.Query);

public void AppendQuery(string name, float value, bool escape = true)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}
public ClientUriBuilder()
{
}

public void AppendQuery(string name, DateTimeOffset value, string format, bool escape = true)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value, format), escape);
}
public void Reset(Uri uri)
{
_uriBuilder = new(uri);
_pathBuilder = new(_uriBuilder.Path);
_queryBuilder = new(_uriBuilder.Query);
}

public void AppendQuery(string name, TimeSpan value, string format, bool escape = true)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value, format), escape);
}
public void AppendPath(string value, bool escape)
{
Argument.AssertNotNullOrWhiteSpace(value, nameof(value));

public void AppendQuery(string name, double value, bool escape = true)
if (escape)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
value = Uri.EscapeDataString(value);
}

public void AppendQuery(string name, decimal value, bool escape = true)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}
PathBuilder.Append(value);
UriBuilder.Path = PathBuilder.ToString();
Comment on lines +40 to +41
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

@ArcturusZhang ArcturusZhang Apr 8, 2024

Choose a reason for hiding this comment

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

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:

  1. The first part included in this change: Azure/autorest.csharp@e5e175f (#4519)
  2. 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

}

public void AppendQuery(string name, int value, bool escape = true)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}
public void AppendQuery(string name, string value, bool escape)
{
Argument.AssertNotNullOrWhiteSpace(name, nameof(name));
Argument.AssertNotNullOrWhiteSpace(value, nameof(value));

public void AppendQuery(string name, long value, bool escape = true)
if (QueryBuilder.Length is 0)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
QueryBuilder.Append('?');
}

public void AppendQuery(string name, TimeSpan value, bool escape = true)
else
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
QueryBuilder.Append('&');
}

public void AppendQuery(string name, byte[] value, string format, bool escape = true)
if (escape)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value, format), escape);
value = Uri.EscapeDataString(value);
}

public void AppendQuery(string name, Guid value, bool escape = true)
{
AppendQuery(name, ModelSerializationExtensions.TypeFormatters.ConvertToString(value), escape);
}
QueryBuilder.Append(name);
QueryBuilder.Append('=');
QueryBuilder.Append(value);
}

public void AppendQueryDelimited<T>(string name, IEnumerable<T> value, string delimiter, bool escape = true)
public Uri ToUri()
{
if (_pathBuilder is not null)
{
var stringValues = value.Select(v => ModelSerializationExtensions.TypeFormatters.ConvertToString(v));
AppendQuery(name, string.Join(delimiter, stringValues), escape);
UriBuilder.Path = _pathBuilder.ToString();
}

public void AppendQueryDelimited<T>(string name, IEnumerable<T> value, string delimiter, string format, bool escape = true)
if (_queryBuilder is not null)
{
var stringValues = value.Select(v => ModelSerializationExtensions.TypeFormatters.ConvertToString(v, format));
AppendQuery(name, string.Join(delimiter, stringValues), escape);
UriBuilder.Query = _queryBuilder.ToString();
}

public Uri ToUri()
{
if (_pathBuilder != null)
{
UriBuilder.Path = _pathBuilder.ToString();
}

if (_queryBuilder != null)
{
UriBuilder.Query = _queryBuilder.ToString();
}

return UriBuilder.Uri;
}
return UriBuilder.Uri;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ internal static class TypeFormatters
public static string ToString(DateTime value, string format) => value.Kind switch
{
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.")
Copy link
Copy Markdown

@ArcturusZhang ArcturusZhang Apr 2, 2024

Choose a reason for hiding this comment

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

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

};

public static string ToString(DateTimeOffset value, string format) => format switch
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,15 +205,15 @@ AudioSegment IPersistableModel<AudioSegment>.Create(BinaryData data, ModelReader
string IPersistableModel<AudioSegment>.GetFormatFromOptions(ModelReaderWriterOptions options) => "J";

/// <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>
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

internal static AudioSegment FromResponse(PipelineResponse response)
{
using var document = JsonDocument.Parse(response.Content);
return DeserializeAudioSegment(document.RootElement);
}

/// <summary> Convert into a Utf8JsonRequestBody. </summary>
internal virtual BinaryContent ToBinaryBody()
/// <summary> Convert into a BinaryContent. </summary>
internal virtual BinaryContent ToBinaryContent()
{
return BinaryContent.Create(this, new ModelReaderWriterOptions("W"));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,15 +164,15 @@ CreateSpeechRequest IPersistableModel<CreateSpeechRequest>.Create(BinaryData dat
string IPersistableModel<CreateSpeechRequest>.GetFormatFromOptions(ModelReaderWriterOptions options) => "J";

/// <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>
internal static CreateSpeechRequest FromResponse(PipelineResponse response)
{
using var document = JsonDocument.Parse(response.Content);
return DeserializeCreateSpeechRequest(document.RootElement);
}

/// <summary> Convert into a Utf8JsonRequestBody. </summary>
internal virtual BinaryContent ToBinaryBody()
/// <summary> Convert into a BinaryContent. </summary>
internal virtual BinaryContent ToBinaryContent()
{
return BinaryContent.Create(this, new ModelReaderWriterOptions("W"));
}
Expand Down
Loading