-
Notifications
You must be signed in to change notification settings - Fork 341
Add stable web_search support and separate factory for web_search_preview #815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add stable web_search support and separate factory for web_search_preview #815
Conversation
jsquire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! The only thing that we may want to do is ask Chris and KC their opinions on the name for usePreview. It makes perfect sense to me.
The only feedback I have is that we typical name bool flags something like |
…hreja_AddStableWebSearchTool
|
I talked to Shivangi offline. I'm realizing now that one issue with this approach is that it doesn't fully work for outputs (or more specifically, for deserialization). That is because the following code would return an "unknown tool" instead of the openai-dotnet/src/Generated/Models/Responses/ResponseTool.Serialization.cs Lines 79 to 80 in ba45b1f
One possible fix is to customize the deserialization code to make it understand both "web_search" and "web_search_preview". However, I think it would be a little inconvenient having to maintain this code manually, especially given how common it is for new tools to be added. Based on this, I'm tempted to suggest that we should add a new |
|
Yes, agreed. Rather than maintaining custom deserialization, I think we should generate two types public static WebSearchTool CreateWebSearchTool(
WebSearchToolLocation userLocation = null,
WebSearchToolContextSize? searchContextSize = null);
public static WebSearchPreviewTool CreateWebSearchPreviewTool(
WebSearchToolLocation userLocation = null,
WebSearchToolContextSize? searchContextSize = null); |
jsquire
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd wait for @joseharriaga to be the authoritative reviewer, but it looks reasonable to me.
This PR adds support for the stable web search tool and separate factory for preview web search tool.
Ref issue: #811