[OpenAI] Update ChatRequestMessage class family and unit tests #41928
Conversation
There was a problem hiding this comment.
Sorry for the comment spread, I am approving because this looks really good :) Thanks! Just to summarize my comments that I believe would be good to apply to all ChatRequest*Message types:
- Make the constructor accepting
BinaryDataprivate - Use
this(BinaryData.from...);where possible to get branch coverage - Don't throw in the
fromJsonmethods, even though they shouldn't generally be called for these types, we might corner ourselves by being too strict in this scenario (i.e., lack of support parity between Azure and non-Azure request message types, in the future)
| } else { | ||
| throw new IllegalStateException("Unexpected 'content' type found when deserializing" | ||
| + " ChatRequestAssistantMessage JSON object: " + reader.currentToken()); | ||
| } |
There was a problem hiding this comment.
I think we may want to be less strict here, specially if at some point we don't have parity with non-Azure OpenAI. WDYT? Also, this might be a non issue, since this class seems to be used only for request and will therefore never be deserialized.
There was a problem hiding this comment.
I would prefer to be strict as we know other token types should not be serialized. I would leave it as it is for now and we can update it later when "parity with Non-Azure OpenAI" raised.
| */ | ||
| @Generated | ||
| public ChatRequestAssistantMessage(BinaryData content) { |
There was a problem hiding this comment.
We may want to make this constructor private, seeing that you have already added a couple of convenience ctxs with types that are more user friendly.
...penai/azure-ai-openai/src/main/java/com/azure/ai/openai/models/ChatRequestSystemMessage.java
Show resolved
Hide resolved
...penai/azure-ai-openai/src/main/java/com/azure/ai/openai/models/ChatRequestSystemMessage.java
Show resolved
Hide resolved
| this.content = BinaryData.fromString(content); | ||
| this.content = content == null ? null : BinaryData.fromString(content); | ||
| this.stringContent = content; | ||
| this.chatMessageContentItem = null; |
There was a problem hiding this comment.
Should we do this(BinaryData.fromString(content)) so we get branch coverage for the generated constructor as well?
There was a problem hiding this comment.
We can't. As the other final properties can't not be assigned.
mssfang
requested review from
alzimmermsft,
srnagar,
g2vinay,
conniey,
rujche,
Netyyyy,
saragluna and
moarychan
as code owners
This PR includes:
Update ChatRequestMessages class families,
ChatRequestSystemMessage,ChatRequestAssistantMessage,ChatRequestToolMessageclasses to be able to support forChatRequestSystemMessagecontent: | string | ChatMessageTextContentItem[]ChatRequestAssistantMessagecontent: | string | ChatMessageTextContentItem[] | ChatMessageRefusalContentItem[] | null (only Assistant can be null)ChatRequestToolMessagecontent: | string | ChatMessageTextContentItem[]Also, re-record all tests