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

feat: add support for o1 models in openai and azure #368

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

sheldonhull
Copy link
Contributor

Add support for OpenAI o1 models by using max_completion_tokens instead of max_tokens.

  • mods.go

    • Add a check in the startCompletionCmd function to determine if the model is an o1 model and set the max_completion_tokens parameter accordingly.
  • config.go

    • Add a new field MaxCompletionTokens to the Config struct to store the value for the max_completion_tokens parameter.
  • config_template.yml

    • Add entries for o1-preview and o1-mini models under the openai section with max-input-chars set to 128000.
    • Add aliases for o1-preview and o1-mini models.
    • Add entries for o1-preview and o1-mini models under the azure section with max-input-chars set to 128000.
    • Add aliases for o1-preview and o1-mini models under the azure section.

Add support for OpenAI o1 models by using `max_completion_tokens` instead of `max_tokens`.

* **mods.go**
  - Add a check in the `startCompletionCmd` function to determine if the model is an o1 model and set the `max_completion_tokens` parameter accordingly.

* **config.go**
  - Add a new field `MaxCompletionTokens` to the `Config` struct to store the value for the `max_completion_tokens` parameter.

* **config_template.yml**
  - Add entries for `o1-preview` and `o1-mini` models under the `openai` section with `max-input-chars` set to 128000.
  - Add aliases for `o1-preview` and `o1-mini` models.
  - Add entries for `o1-preview` and `o1-mini` models under the `azure` section with `max-input-chars` set to 128000.
  - Add aliases for `o1-preview` and `o1-mini` models under the `azure` section.
@sheldonhull
Copy link
Contributor Author

Note, this is placeholder PR to jump start via github workspaces spec, but I haven't gone through and changed 32768 as max tokens and some other steps. Figured I'd get it up as a draft to get it on the radar and can improve soon. So take this as a true draft for investigating fix for #367

@sheldonhull
Copy link
Contributor Author

@caarlos0 so overall this seemed correct as long as I fix tokens? I generated this and didn't have time to come back round then noticed your feedback was applied. I can try and get this wrapped up today the. If you have no issues with approach.

@caarlos0
Copy link
Member

caarlos0 commented Nov 7, 2024

@caarlos0 so overall this seemed correct as long as I fix tokens? I generated this and didn't have time to come back round then noticed your feedback was applied. I can try and get this wrapped up today the. If you have no issues with approach.

yes, seems correct to me, at least at first sight. Happy to help test/etc or if you need anything 🙏🏻

@bashbunni bashbunni added the enhancement New feature or request label Dec 4, 2024
Add support for OpenAI o1 models by using `max_completion_tokens` instead of `max_tokens`.

* **mods.go**
  - Add a check in the `startCompletionCmd` function to determine if the model is an o1 model and set the `max_completion_tokens` parameter accordingly.

* **config.go**
  - Add a new field `MaxCompletionTokens` to the `Config` struct to store the value for the `max_completion_tokens` parameter.

* **config_template.yml**
  - Add entries for `o1-preview` and `o1-mini` models under the `openai` section with `max-input-chars` set to 128000.
  - Add aliases for `o1-preview` and `o1-mini` models.
  - Add entries for `o1-preview` and `o1-mini` models under the `azure` section with `max-input-chars` set to 128000.
  - Add aliases for `o1-preview` and `o1-mini` models under the `azure` section.
@sheldonhull
Copy link
Contributor Author

sheldonhull commented Jan 10, 2025

Trying to goreleaser build and ran into this:

  ⨯ build failed after 0s                    error=yaml: unmarshal errors:
  line 7: field variables not found in type config.Project

I tried copying part of meta/goreleaser config for just the build in and still running into it.
Had to strip out the variables and add project name for it work.

Is there standard approach for no pro members to be able to jump into these and build using goreleaser? I tried just go build directly and also didn't work.

Once I stripped all those out and pasted directly here I could run goreleaser build --snapshot.

edit: just ran into:

⨯ build failed after 5s                    error=template: failed to apply "{{ with .Var.main }}{{ . }}{{ else }}.{{ end }}": map has no entry for key "Var"

Fixed by just putting in ., but then ran into other vars not set.

@caarlos0
Copy link
Member

mods goreleaser config requires goreleaser pro

that said, you can still build it with go build, and generally don't need to build all binaries locally

@sheldonhull
Copy link
Contributor Author

sheldonhull commented Jan 13, 2025

@caarlos0 I tried goreleaser because go build wasn't working. There's no taskfile/mage/make to build either. I'm relooking to see what I'm missing and will update this comment if I figure it out.

Ok, I needed go run ./... and then it worked. My bad :-p

CleanShot 2025-01-13 at 12 14 42

@sheldonhull
Copy link
Contributor Author

sheldonhull commented Jan 13, 2025

Looking further, running local test I get:

This model has beta-limitations, user and assistant messages only, system messages are not supported.

Attempting with --role user and --role assistant isn't working either saying those roles don't exist.
In reviewing the code, I see this is more of a preset prompt role for mods, not providing the ability to set OpenAI role type of system, user, or assistant.

@caarlos0
Copy link
Member

roles is a bit confusing because it has a different meaning in mods.

run mods --settings, you'll see the roles section.

there you can define custom roles and their instructions, e.g.:

roles:
  "dwight":
    - act as dwight shrute, assistant to the regional manager, from the "The Office" TV show

and then you can:

$ mods -R dwight "whats your evil plan?"

  Ah, so you want to know my evil plan, do you? Well, a true master never reveals
  all his secrets. However, rest assured, I have several contingency plans in
  place for any scenario that might arise within the office. Whether it's ensuring
  total loyalty to Dunder Mifflin, winning "Salesman of the Year," or
  outperforming Jim in all aspects of office warfare, I've got things well under
  control.

  But remember, as Assistant to the Regional Manager, my primary duty is to serve
  and protect the branch, uphold the standards set by Michael Scott (and now, of
  course, David Wallace), and ensure that paper sales at Dunder Mifflin's Scranton
  branch continue to soar. Anything else you need to know, like how I manage the
  Schrute Farms beet harvest?

@sheldonhull
Copy link
Contributor Author

Yes, I updated comment to reflect that so thanks for confirming I got it right.
So I tried testing to confirm changes are good, but the azure openai access to o1-preview limits api usage. The o1 model supposedly will allow it but it's a per subscription application so I'm on a waiting list. Can't test these changes to confirm it's solid right now for azure openai though it compiles fine.

Do you want this to stay in draft till I can run against azure openai (no ETA), or proceed with what we have already?

@caarlos0
Copy link
Member

I also don't have access to the azure thing. I think if it works on openai we can probably merge it.

BTW have a new release of mods coming soon so if we merge this it'll be in it as well :)

@sheldonhull
Copy link
Contributor Author

@caarlos0

this model has beta-limitations, user and assistant messages only, system messages are not supported.

I can't test it as currently it doesn't support system messages, so I'd have to remap. How would you want to handle this? Just wait until released or merge as is and then remove after all the previews are done?

I see Google create stream has this handled, but that seems to be a permanent part, not a beta part. I also see some other references to o1.

mods/stream.go

Lines 95 to 110 in 34d8469

// Google doesn't support the System role so we need to remove those message
// and, instead, store their content on the `System` request value.
//
// Also, the shape of Google messages is slightly different, so we make the
// conversion here.
messages := []GoogleContent{}
for _, message := range m.messages {
if message.Role == openai.ChatMessageRoleSystem {
parts := []GoogleParts{
{Text: fmt.Sprintf("%s\n", message.Content)},
}
messages = append(messages, GoogleContent{
Role: "user",
Parts: parts,
})

@sheldonhull
Copy link
Contributor Author

I pushed a change for review that does this based on the pattern in Google stream.
I ran locally and it compiled, but says I've exceeded my openai rate limit. I don't use openai right now so if you have the ability to run a quick test against a text file and confirm it works that would be great. I just tried to run something like this

 cat .artifacts/input.txt | go run ./... --api openai --model o1-mini  -p "1.) Analyze the input text and generate 5 essential questions that, when answered, capture the main points and core meaning of the text. 2.) When formulating your questions: a. Address the central theme or argument b. Identify key supporting ideas c. Highlight important facts or evidence d. Reveal the author's purpose or perspective e. Explore any significant implications or conclusions. 3.) Answer all of your generated questions one-by-one in detail. *** What do you think?" --format markdown | gum format

@sheldonhull sheldonhull marked this pull request as ready for review January 13, 2025 22:12
@caarlos0
Copy link
Member

it seems that we actually need to unset max tokens for it to work.

pushed that change in c394fff, tested here and seemed to work fine 🙏🏻

@caarlos0 caarlos0 merged commit e5e4bdd into charmbracelet:main Jan 14, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants