Default assistants from file + models from file #560

drnic · 2024-11-20T08:17:22Z

This PR includes #536

Instead of new Assistants being defined in code, they are defined in assistants.yml.

The PR defines the same Assistants as before, plus a new "GPT-4o Mini" assistant.

Assistants have a unique (per user) slug which is used in urls /assistants/claude-sonnet/messages/new and used to match updates from assistants.yml to existing Assistant.

krschacht · 2024-11-21T01:03:04Z

@drnic I merged in your other PR and it created some merge conflicts. I took a quick look but I'm not 100% sure on the right way to resolve so I don't want to break it.

drnic · 2024-11-21T02:03:37Z

Ok, rebuilt the PR off main.

Question: in Assistant what is Assistant#tools used for? I don't think its currently being used to select or filter the available Toolbox classes. If its unused, I can drop it from the DEFAULT_EXPORT_ONLY list until its used in future.

drnic · 2024-11-21T02:04:34Z

Also, perhaps Assistant#instructions should not be exported to assistants.yml as I think this is where each user's personal "memory" is stored?

drnic · 2024-11-21T02:05:25Z

And, are we using Assistant#external_id yet? What's its future use?

krschacht · 2024-11-21T03:13:48Z

@drnic Ah, currently our Assistants class is a local-only model. But you know how I mentioned that OpenAI has this new API where they persist Assistants: at one point in time I was preparing to implement that API so I added the missing fields. Those fields are to match this schema:

https://platform.openai.com/docs/api-reference/assistants/createAssistant

It's probably worth dropping those columns since they're not actually implemented. I don't like keeping around half-implemented code and we can always add them back if ever deciding to implement the Assistant API.

krschacht · 2024-11-21T03:15:01Z

It looks like this branch is behind main. I'll wait to review until you flag it for me.

drnic · 2024-11-21T03:40:07Z

The branch was rebased/rebuilt against main, but it has some flakey specs. I'll investigate. E.g. this passes locally:

LanguageModel::ExportTest#test_import_from_file_with_existing_models_by_api_name [test/models/language_model/export_test.rb:95]:
"LanguageModel.count" didn't change by 0, but by 1.
Expected: 30
  Actual: 31

bin/rails test test/models/language_model/export_test.rb:78

drnic · 2024-11-21T03:49:51Z

Ok ci green

krschacht · 2024-11-21T03:51:02Z

Ohhh, sorry when I went to diff I saw include Export and other code that was so familiar from the other PR — which was merged into main — I thought it was mistakenly showing me parts of the diff that were incorrect. I see now this is all new code. :)

I just did a pass through the diff and it looks good. Are there any other changes you had planned for it or is it ready to be merged in, from your view?

drnic · 2024-11-21T03:52:38Z

I think it's good as it is. There's always other ideas. But this is handy and it's now obvious where assistants + their associated LLM models come from.

drnic · 2024-11-21T03:53:33Z

I'd do a PR for dropping the unused fields later.

app/models/assistant/export.rb

krschacht

Left one comment inline

…inders

drnic · 2024-11-26T09:59:01Z

The failing system spec is same as on main

drnic · 2024-11-26T21:55:27Z

Please double check the ideas in new migration -- is this a useful help to existing users' to generate slugs that match to the assistants.yml; or is it better to match based on the name only -- if a previous user didn't change the name, then they get the matching slug.

mattlindsey · 2024-11-27T12:06:18Z

Please double check the ideas in new migration

Currently the migration leaves Assistants that I created to slug: nil so they disappear from the UI. Is your idea to create a slug from the Name?
Or is exporting them a required step before running db:migrate?

P.S. This might be a wierd case. The Language Model for my Assistant is based on an API Service that I had added, so that may be a case you need to handle.

drnic · 2024-11-27T12:19:42Z

Currently the migration leaves Assistants that I created to slug: nil so they disappear from the UI. Is your idea to create a slug from the Name?
Or is exporting them a required step before running db:migrate?

In the PR it will add the new slug column and then generate a slug for every assistant based on its name. Except first it tries to find the original Assistants and give them the same slugs that are now in the assistants.yml file.

mattlindsey · 2024-11-27T12:21:55Z

This might be a wierd case. The Language Model for my Assistant is based on an API Service that I had added, so that may be a case you need to handle. Let me know if you need more info.

drnic · 2024-11-27T12:22:12Z

The question I’m trying to solve in the migration — since we don’t already have the slug — what to do with existing Assistants vs the incoming assistants.yml.

some/all users will find new Assistants added from the assistants.yml file (eg gpt-4o mini). But can we minimise this? Or is it fine to create a few extra assistants?

drnic · 2024-11-27T12:24:06Z

This might be a wierd case. The Language Model for my Assistant is based on an API Service that I had added, so that may be a case you need to handle. Let me know if you need more info.

Some people might have a simple 1:1 between an assistant and a language model api_name.

Others might have 10 assistants for gpt-4o, each with different system prompts, ready to behave differently from each other; but using the same language model.

drnic · 2024-11-27T12:26:42Z

So what the migration is doing is finding the oldest assistant for each of the primary language models (gpt-4o, Claude, llama-3.1-70b) and assign them their assistants.yml slug.

The rest get a generated slug from their name.

However, the “original” assistants will have their name overridden by assistants.yml. And that might not be what we want.

drnic · 2024-11-27T12:31:21Z

Maybe assistants.yml should only update the language model/api name for the assistants; but not change the assistant name.

mattlindsey · 2024-11-27T12:36:31Z

Maybe assistants.yml should only update the language model/api name for the assistants; but not change the assistant name.

Cool. I was just saying currently in this PR the slug for my assistant gets nil, but when your done with this PR it should get one based on the Name. Right? I'm a little unclear about your issue regarding names, but hopefully people won't mind too much if a name gets changed (as long as you don't keep changing them back to a default when they want a different name).

drnic · 2024-11-27T19:15:15Z

Cool. I was just saying currently in this PR the slug for my assistant gets nil,

Ah, that’s a bug. The migration should run save! on every assistant which will generate the slug. I’ll try to reproduce.

(as long as you don't keep changing them back to a default when they want a different name).

Yeah that’s the issue - the PR currently will keep renaming the assistants each time you db:prepare; so that’s annoying. I’ll update that.

krschacht · 2024-11-27T23:55:37Z

@mattlindsey what's the name of your assistant which got a nil slug?

krschacht · 2024-11-27T23:59:12Z

test/controllers/password_resets_controller_test.rb

+    # Test can flap on CI if we don't clear the queue
+    clear_enqueued_jobs
+
+    # set the user agent in the request headers


thank you!! I saw this flappy test a couple times in dev but wasn't able to figure it out yet

mattlindsey · 2024-11-28T00:02:04Z

@mattlindsey what's the name of your assistant which got a nil slug?

I just checked again and the slug is not nil. Sorry about that.
There must be another reason it's not showing up in the UI.
Below is from the console. Maybe if I send the model and api records too, you can figure out why.

#<Assistant:0x0000000127c56048
  id: 4,
  user_id: 1,
  name: "Ollama",
  description: "Meta llama7b on ollama",
  instructions: "",
  tools: [],
  created_at: "2024-11-25 10:29:16.222762000 -0600",
  updated_at: "2024-11-27 06:08:57.487161000 -0600",
  language_model_id: 34,
  deleted_at: "2024-11-26 10:00:19.987117000 -0600",
  external_id: nil,
  slug: "ollama">]

mattlindsey · 2024-11-28T00:04:58Z

 #<LanguageModel:0x0000000136d15b50
  id: 34,
  position: 34,
  api_name: "llama3.1:8b",
  name: "Meta Llama 3.1 8b",
  supports_images: false,
  created_at: "2024-11-25 10:34:39.007164000 -0600",
  updated_at: "2024-11-25 11:16:27.141688000 -0600",
  deleted_at: nil,
  user_id: 1,
  api_service_id: 4,
  supports_tools: false,
  input_token_cost_cents: 0.5e-5,
  output_token_cost_cents: 0.8e-5,
  best: false,
  supports_system_message: true>,
  
   #<APIService:0x0000000134a92f88
  id: 4,
  user_id: 1,
  name: "Ollama",
  driver: "openai",
  url: "http://localhost:11434/v1/",
  token: [FILTERED],
  deleted_at: nil,
  created_at: "2024-11-25 10:33:26.337918000 -0600",
  updated_at: "2024-11-25 11:07:46.365756000 -0600">]

mattlindsey · 2024-11-28T00:09:43Z

@krschacht Damn it. It is there fine. Sorry for the wild goose chase!

krschacht · 2024-11-28T00:11:52Z

app/controllers/messages_controller.rb

@@ -73,7 +73,7 @@ def set_conversation
  end

  def set_assistant
-    @assistant = Current.user.assistants_including_deleted.find_by(id: params[:assistant_id])
+    @assistant = Current.user.assistants_including_deleted.find_by(slug: params[:assistant_id])


I was a bit surprised that this worked because I did not see you change a corresponding link_to in order to generate a url that contains this slug. Is that some default rails helper? I tried asking gpt4 but it claimed not so I don't get how the URLs are now using slug rather than id.

See Assistant#to_param which is magically used by rails routes helpers

Oh, clever! But now I can't figure out why a newly added assistant is not working? Based on that implementation I would expect it to recognize any slug. Let's move over to this thread: #525 (comment)

krschacht · 2024-11-28T00:13:31Z

@mattlindsey no problem at all, it's great that you were previewing the PR and trying it out.

@drnic I just finished reviewing and testing locally and I think it's good to go! I'm going to merge in now.

sorry about this flappy system test. I think Justin might take a look at it this week when he wraps up his PR.

drnic force-pushed the default-assistants-from-file branch from 3359e59 to f16de77 Compare November 21, 2024 02:01

drnic force-pushed the default-assistants-from-file branch from f16de77 to db2049c Compare November 21, 2024 02:31

drnic force-pushed the default-assistants-from-file branch from db2049c to 662998b Compare November 21, 2024 03:44

krschacht reviewed Nov 24, 2024

View reviewed changes

app/models/assistant/export.rb Show resolved Hide resolved

krschacht requested changes Nov 24, 2024

View reviewed changes

Import Assistants from assistants.yml

2924648

drnic force-pushed the default-assistants-from-file branch from 662998b to 2924648 Compare November 26, 2024 08:35

drnic added 7 commits November 26, 2024 19:00

Add slug to Assistant

1db62f6

Generate slug if missing on save

9773cbe

Look up language model for user

c6311b4

Match first/oldest LanguageModel to api_name; generate slugs for rema…

b2cda7c

…inders

Append --1,--2 to slug to make unique

2119bdf

Don't generate slug if name missing

f4ec87a

Drop Assistant #tools + #external_id from assistants.yml export

2512211

drnic requested a review from krschacht November 26, 2024 09:29

Correctly import by slug

c8d417a

drnic force-pushed the default-assistants-from-file branch from b5621a2 to c8d417a Compare November 26, 2024 09:46

Use slug for Assistant param

dabb651

Can edit assistant slug

0fc9c57

krschacht reviewed Nov 27, 2024

View reviewed changes

krschacht reviewed Nov 28, 2024

View reviewed changes

krschacht merged commit c910350 into AllYourBot:main Nov 28, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default assistants from file + models from file #560

Default assistants from file + models from file #560

drnic commented Nov 20, 2024 •

edited

Loading

krschacht commented Nov 21, 2024

drnic commented Nov 21, 2024

drnic commented Nov 21, 2024

drnic commented Nov 21, 2024

krschacht commented Nov 21, 2024

krschacht commented Nov 21, 2024

drnic commented Nov 21, 2024

drnic commented Nov 21, 2024

krschacht commented Nov 21, 2024

drnic commented Nov 21, 2024 •

edited

Loading

drnic commented Nov 21, 2024

krschacht left a comment

drnic commented Nov 26, 2024 •

edited

Loading

drnic commented Nov 26, 2024

mattlindsey commented Nov 27, 2024 •

edited

Loading

drnic commented Nov 27, 2024

mattlindsey commented Nov 27, 2024

drnic commented Nov 27, 2024

drnic commented Nov 27, 2024

drnic commented Nov 27, 2024

drnic commented Nov 27, 2024

mattlindsey commented Nov 27, 2024 •

edited

Loading

drnic commented Nov 27, 2024

krschacht commented Nov 27, 2024

krschacht Nov 27, 2024

mattlindsey commented Nov 28, 2024

mattlindsey commented Nov 28, 2024

mattlindsey commented Nov 28, 2024 •

edited

Loading

krschacht Nov 28, 2024

drnic Nov 28, 2024 •

edited

Loading

krschacht Nov 28, 2024

krschacht commented Nov 28, 2024

Default assistants from file + models from file #560

Default assistants from file + models from file #560

Conversation

drnic commented Nov 20, 2024 • edited Loading

krschacht commented Nov 21, 2024

drnic commented Nov 21, 2024

drnic commented Nov 21, 2024

drnic commented Nov 21, 2024

krschacht commented Nov 21, 2024

krschacht commented Nov 21, 2024

drnic commented Nov 21, 2024

drnic commented Nov 21, 2024

krschacht commented Nov 21, 2024

drnic commented Nov 21, 2024 • edited Loading

drnic commented Nov 21, 2024

krschacht left a comment

Choose a reason for hiding this comment

drnic commented Nov 26, 2024 • edited Loading

drnic commented Nov 26, 2024

mattlindsey commented Nov 27, 2024 • edited Loading

drnic commented Nov 27, 2024

mattlindsey commented Nov 27, 2024

drnic commented Nov 27, 2024

drnic commented Nov 27, 2024

drnic commented Nov 27, 2024

drnic commented Nov 27, 2024

mattlindsey commented Nov 27, 2024 • edited Loading

drnic commented Nov 27, 2024

krschacht commented Nov 27, 2024

krschacht Nov 27, 2024

Choose a reason for hiding this comment

mattlindsey commented Nov 28, 2024

mattlindsey commented Nov 28, 2024

mattlindsey commented Nov 28, 2024 • edited Loading

krschacht Nov 28, 2024

Choose a reason for hiding this comment

drnic Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

krschacht Nov 28, 2024

Choose a reason for hiding this comment

krschacht commented Nov 28, 2024

drnic commented Nov 20, 2024 •

edited

Loading

drnic commented Nov 21, 2024 •

edited

Loading

drnic commented Nov 26, 2024 •

edited

Loading

mattlindsey commented Nov 27, 2024 •

edited

Loading

mattlindsey commented Nov 27, 2024 •

edited

Loading

mattlindsey commented Nov 28, 2024 •

edited

Loading

drnic Nov 28, 2024 •

edited

Loading