Skip to content
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

OpenAI: Add o1/o1-mini to chat model enum #1361

Closed
wants to merge 1 commit into from

Conversation

PARK-afk
Copy link
Contributor

Thank you for taking time to contribute this pull request!
You might have already read the [contributor guide][1], but as a reminder, please make sure to:

  • Sign the contributor license agreement
  • Rebase your changes on the latest main branch and squash your commits
  • Add/Update unit tests as needed
  • Run a build and make sure all tests pass prior to submission

@@ -183,6 +183,26 @@ public OpenAiApi(String baseUrl, String apiKey, MultiValueMap<String, String> he
*/
public enum ChatModel implements ChatModelDescription {

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These models are a bit different compared to the rest. For example, they don't support function calling nor system messages. I would suggest considering how to convey this information to warn users that by switching model value they might get a broken application. It might be worth considering whether adding them to this enum is necessary. Even if they're not added to the enum, it's always possible to use these new models, either via configuration properties or via the ChatModel/ChatClient APIs.

Copy link
Member

@markpollack markpollack Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point. we need to think about it. in anycase, i merged the enums in the interest of having folks explore from a common reference point for api discovery. that said, it will be interesting to see how much openai break their contracts. eg. i don't think temperature or some other options apply for this model. the landscape just gets more complex...

@markpollack
Copy link
Member

merged in 40eaa99

will create an issue to discuss how 'common options' are not getting i'll defined.

@dafriz
Copy link
Contributor

dafriz commented Sep 17, 2024

Note that the commit that got merged has the wrong values - 40eaa99 . It was his follow up commit that had the names that match OpenAI docs - 0bd27e4

See https://platform.openai.com/docs/models/o1 - they start with o1, not gpt.

@markpollack
Copy link
Member

ah, I got confused. I haven't yet played with the model myself. Thanks for that.

@markpollack
Copy link
Member

I've updated the enums to be


		/**
		 * Points to the most recent snapshot of the o1 model:o1-preview-2024-09-12
		 */
		O1_PREVIEW("o1-preview"),
		/**
		 * Latest o1 model snapshot
		 */
		O1_PREVIEW_2024_09_12("o1-preview-2024-09-12"),
		/**
		 * Points to the most recent o1-mini snapshot:o1-mini-2024-09-12
		 */
		O1_MINI("o1-mini"),
		/**
		 * Latest o1-mini model snapshot
		 */
		O1_MINI_2024_09_12("o1-mini-2024-09-12")

@markpollack
Copy link
Member

closing this PR and created #1378 for deeper analysis of rationalizing chatoptions, in particular due to how different the o1 model is regarding options and also functionality (e.g. no system messages)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants