-
Notifications
You must be signed in to change notification settings - Fork 237
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
Refresh lang models for all users #536
base: main
Are you sure you want to change the base?
Conversation
Update it from local DB with: rails models:export
3e3f195
to
d1c84bb
Compare
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 know you're still working on this, but I did a quick pass through since I had some time and shared a few thoughts. It's looking good! Thanks so much for tackling this!
@@ -51,6 +51,7 @@ shared: | |||
default_openai_key: <%= ENV["DEFAULT_OPENAI_KEY"] %> | |||
default_anthropic_key: <%= ENV["DEFAULT_ANTHROPIC_KEY"] %> | |||
default_groq_key: <%= ENV["DEFAULT_GROQ_KEY"] %> | |||
lang_models_url: <%= ENV.fetch("LANG_MODELS_URL", "https://raw.githubusercontent.com/AllYourBot/hostedgpt/refs/heads/main/models.json") %> |
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.
For all new features, I've been trying to support them within options.yml. I've really come to like the rails pattern of getting to store secrets and configs in the bin/rails credentials:edit
. Is that too unconventional?
To continue support of it I think this line would need to be:
<%= ENV["LANG_MODELS_URL"] || default_to(:lang_models_url) || "https://raw.githubusercontent.com/AllYourBot/hostedgpt/refs/heads/main/models.json" %>
supports_images: true | ||
supports_tools: true | ||
input_token_cost_cents: '0.00025' | ||
output_token_cost_cents: '0.001' |
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.
Just like you left the quotes off the strings, I'm pretty sure you don't need quotes around floats in a yml file
@@ -47,11 +49,51 @@ def created_by_current_user? | |||
user == Current.user | |||
end | |||
|
|||
def api_service_name | |||
api_service.name | |||
end |
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.
How about delegate :name, to: :api_service, prefix: true
and then pull it up to the other delegate
a few lines above
warn "Failed to import '#{model[:api_name]}': #{e.message} for #{model.inspect}" | ||
end | ||
end | ||
end |
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.
Can we put these two methods and the as_json
into a concern? I think their ancillary enough that they don't need to be in the core model file. I know this model doesn't have any other concerns yet but it's just about ready for it.
args.with_defaults(path: Rails.root.join("models.yaml")) | ||
users = User.all | ||
LanguageModel.import_from_file(path: args[:path], users:) | ||
end |
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 call, it's nice to have these helper tasks
@@ -29,14 +29,14 @@ This project is led by an experienced rails developer, but I'm actively looking | |||
- [Deploy the app on Heroku](#deploy-the-app-on-heroku) | |||
- [Deploy on your own server](#deploy-on-your-own-server) | |||
- [Configure optional features](#configure-optional-features) | |||
- [Give assistant access to your Google apps](#configuring-google-tools) | |||
- [Configuring Google Tools](#configuring-google-tools) |
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 for catching the broken anchors below. But on this change, since our readme has gotten so long, I like the more descriptive "Give assistant access to your Google apps" on this one. It gives people a vague idea of what's possible whereas I don't the more succinct "Configuring Google Tools" hints at enough.
|
||
```plain | ||
rails models:export[tmp/models.json] | ||
``` |
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 for adding this explanation. I am wondering though if we can add some smart defaults so that a new user can safely ignore this and still get reasonable behavior. Can we make it so that bin/dev
also runs models:import
on startup? It already runs migrations and bundle upon startup so I've been leaning into it.
We have a fair number of people using this app who are only mildly technical, or are technical but don't know rails, so it's been nice to keep the day-to-day running of it super simple.
But actually, now that I write all this, I don't think people will reliably restart their server. It's not like fly, render, heroku do an automatic nightly restart or anything. Now I'm on the fence... I think I still learn towards doing it on startup, since it's a bit of a catch all. But we'll also add a button to the Settings at some point. And maybe later on we could even add a nightly sync of models:import in the queue. (Scope creep for now, I'm just thinking out loud.)
@@ -4,12 +4,14 @@ class LanguageModel < ApplicationRecord | |||
BEST_CLAUDE = "claude-best" | |||
BEST_GROQ = "groq-best" | |||
|
|||
# TODO: infer these from models.yaml |
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.
Agreed, it's nice to eliminate this config
BEST_MODELS = { | ||
BEST_GPT => "gpt-4o-2024-08-06", | ||
BEST_CLAUDE => "claude-3-5-sonnet-20240620", | ||
BEST_GROQ => "llama3-70b-8192", | ||
} | ||
|
||
# TODO: what are these for? |
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.
The app now tracks your usage $. Currently, when you click "..." on a conversation you see this:
Soon I want to add it so that when you click on your profile pic (button left) then it shows something equivalent for the month-to-date across all conversations.
But... (note to self) these two things should probably be moved to a separate concern of their own. They're not so important as to be so prominent in this model.
#535
In this PR, we provide a rake task
rails models:import
to create/updateLanguageModel
for allUser
from themodels.yaml
file in the root of the repo.This
models.yaml
file can be manually modified, or can be regenerated from the first User in the local DB withrails models:export
ffi 1.17.0
change needed for macos devcreate_initial_assistants_etc