-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix structured output dispatch and settings #14811
Conversation
fixed #14810 Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
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.
Thank you! Seems to work fine, except for a typo with gpt-4 (see inline).
I tested this with the Terminal agent and setting different OpenAI models to be used for this agent.
@@ -167,3 +170,4 @@ export class OpenAiFrontendApplicationContribution implements FrontendApplicatio | |||
|
|||
const openAIModelsWithDisabledStreaming = ['o1-preview', 'o1-mini']; | |||
const openAIModelsSupportingDeveloperMessages = ['o1-preview', 'o1-mini']; | |||
const openAIModelsWithoutStructuredOutput = ['o1-preview', 'gpt-4-turbo', 'gtp-4', 'gpt-3.5-turbo']; |
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.
By turning the whitelist into a blacklist, accidentally 'gpt-4o-2024-05-13' is now considered to support structured output, which it infact does not. Also I believe 'o1-mini' does not support structured output (https://platform.openai.com/docs/models#o1, just o1, but I might misinterpret the docs, haven't tested it).
I think expecting structured output, while there is no support for it currently yields an error in Theia AI at the moment. So I wonder whether going with blacklisting is a good idea. People can always use a custom model to work around that (in both directions) though, so I don't have hard feelings either way.
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.
Also gtp-4 -> gpt-4
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.
Good points!!
- typo is fixed
- I added "gpt-4o-2024-05-13" and "o1-mini"
I decided for a blacklist because it seems that all newer models are supporting structured output (with the exception of early o1 stream), so I believe this is less maintenance in the future. For the blacklisted ones, I tried to cover everything that might still be used and that we had previously in our default list. I now added 'gpt-4o-2024-05-13" even though I hope nobody is using this anymore.
In any case, if a model is configured wrongly, there will be an error and you can use a custom configuration. This will actually at the moment only hit users using an agent that uses structured output (the terminal is the only in the Theia IDE) AND manage to select an OpenAI LLM that does not support this, which I find unlikely. Tool builders have this under control anyways.
Signed-off-by: Jonas Helming <jhelming@eclipsesource.com>
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.
Thanks, looks good to me! 👍
fixed #14810
What it does
How to test
Use the terminal agent with 4o and o1-preview and verify with a debugger that structured output is used only for 4o
Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers