Preliminary support for Assistants streaming#26
Conversation
|
|
||
| internal class MessageRoleSerialization | ||
| { | ||
| public static MessageRole DeserializeMessageRole(JsonElement jsonElement, ModelReaderWriterOptions options = default) |
There was a problem hiding this comment.
Why are we not using one of the SCM serialization APIs to deserialize?
There was a problem hiding this comment.
I'm not sure how to handle that with a plain enum; I could pass this all through to the generated, internal extensible enum, but that feels unnecessarily heavy. Is there a pattern I can follow to not manually implement this?
There was a problem hiding this comment.
@m-nash, what's the best way to support enum serialization?
| } | ||
| else | ||
| { | ||
| throw new NotImplementedException(roleName); |
There was a problem hiding this comment.
Are there other values that we dont implement or we just don't know of other values? If the later, we should not NIE. We should either add Other value to MessageRole or throw some other exception.
There was a problem hiding this comment.
This is exhaustive per what's specified today: https://github.com/openai/openai-openapi/blob/28a300c5784415af66f81b2acc0db182f6eb3bbd/openapi.yaml#L8221
We wouldn't anticipate being able to effectively deal with novel roles in the convenience layer without a code update, so a non-extensible enum with the two known values feels appropriate here. Do you agree?
There was a problem hiding this comment.
I saw the ApiView comment related to this. At a minimum, I'll add an "Other"/"Unknown" as a default.
| foreach (var item in _serializedAdditionalRawData) | ||
| { | ||
| writer.WritePropertyName(item.Key); | ||
| #if NET6_0_OR_GREATER |
There was a problem hiding this comment.
this might be a common pattern. Should we add a helper for writing raw values somewhere and then just use this helper.
There was a problem hiding this comment.
100% agreed; there's a lot more here in the custom code folder than we'd like, but this (along with several other patterns) would be great candidates for codegen-level refactor -- even if just for readability.
There was a problem hiding this comment.
@annelo-msft, do we have a convention for how to organize such polyfills in our sources?
There was a problem hiding this comment.
The short answer is that we do in Core and ClientModel, but in client code we're either using shared source or the generator is writing the #if sequence directly.
I agree that it would be nice to have a better answer - let me flag this as work to consider as part of Azure/azure-sdk-for-net#28369.
3 participants
No description provided.