Add ChatModelFactory class to OpenAI namespace for easier mocking #531
Conversation
Co-authored-by: KrzysztofCwalina <[email protected]>
…age details Co-authored-by: KrzysztofCwalina <[email protected]>
…tructors Co-authored-by: KrzysztofCwalina <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
KrzysztofCwalina
requested review from
ShivangiReja,
joseharriaga and
trrwilson
as code owners
src/Custom/ChatModelFactory.cs
Outdated
| { | ||
| /// <summary> Initializes a new instance of <see cref="OpenAI.Chat.ChatCompletion"/>. </summary> | ||
| /// <returns> A new <see cref="OpenAI.Chat.ChatCompletion"/> instance for mocking. </returns> | ||
| public static ChatCompletion ChatCompletion( |
There was a problem hiding this comment.
@KrzysztofCwalina - I was wondering, why do we have to write the model factory methods manually? Shouldn't these be generated by the code gen?
There was a problem hiding this comment.
I am not sure. Josh would know
There was a problem hiding this comment.
@JoshLove-msft / @jorgerangel-msft / @joseharriaga - Do you know why model factory methods aren't being generated for these types?
There was a problem hiding this comment.
The implementation is calling into a different model factory overload? Isn't that one being generated? What is the purpose of this new overload?
There was a problem hiding this comment.
Oh, this is a namespace specific model factory. The generator doesn't support this currently. There is one model factory per library.
There was a problem hiding this comment.
Wait, maybe the PR I was going before the last GA merged? I thought we punted it and this was the attempt to restart. Let's me investigate
There was a problem hiding this comment.
For reference, some of these custom methods are already being generated in the root namespace: https://github.com/openai/openai-dotnet/blob/main/src/Generated/OpenAIModelFactory.cs#L296
There was a problem hiding this comment.
@copilot, dont add model factory methods (or classes) if they aready exist. Only if they don't as in this PR description
There was a problem hiding this comment.
You're correct. I've removed the entire ChatModelFactory class since OpenAIChatModelFactory already provides all the necessary factory methods for Chat-related types. Commit ea57bbc removes the redundant factory class and its associated tests.
src/Custom/ChatModelFactory.cs
Outdated
| { | ||
| /// <summary> Initializes a new instance of <see cref="OpenAI.Chat.ChatCompletion"/>. </summary> | ||
| /// <returns> A new <see cref="OpenAI.Chat.ChatCompletion"/> instance for mocking. </returns> | ||
| public static ChatCompletion ChatCompletion( |
There was a problem hiding this comment.
Wait, maybe the PR I was going before the last GA merged? I thought we punted it and this was the attempt to restart. Let's me investigate
src/Custom/ChatModelFactory.cs
Outdated
| { | ||
| /// <summary> Initializes a new instance of <see cref="OpenAI.Chat.ChatCompletion"/>. </summary> | ||
| /// <returns> A new <see cref="OpenAI.Chat.ChatCompletion"/> instance for mocking. </returns> | ||
| public static ChatCompletion ChatCompletion( |
There was a problem hiding this comment.
@copilot, dont add model factory methods (or classes) if they aready exist. Only if they don't as in this PR description
…y exists Co-authored-by: KrzysztofCwalina <[email protected]>
|
It seems like the other PR went in before GA |
All namespaces have model factory types, e.g. OpenAIModelFactory
Reviews the model factory methods to make sure that all output models can be created and initialized. If some cannot, i.e. there is no model factory, the type does not have public ctor, or settable public properties, add a model factory method for this model type.