Create & manage LanguageModels and APIService (+add Llama 3) #389
Conversation
There was a problem hiding this comment.
I just did a quick skim through this and flagged a few small items that I noticed.
To reply to your other comment:
Thanks for the feedback.
It's a good point that it looks heavy, that language models and API services are separated. Indeed I had them combined initially. Then I looked at the database schema and it wasn't normalized; then I used the app the next day and it was annoying to look up the port and access token. So once one has a API server setup I think it's natural to have that reflected in one database table, and one UI section. Its often effort to locate the access-token (again)
I also agree they should be separate. My comment was slightly different. I was proposing we do keep them the same, but currently you edit an Assistant and you assign a LanguageModel and an API. I was proposing instead that from Assistant you just assign a LanguageModel, and then you edit a LanguageModel to assign it to an API. But APIs would still be their own entities and they would each of their own section within the Settings and you could still assign multiple LanguageModels to the same API so you wouldn’t have to keep specifying the port & token repeatedly.
I feel the assistants should not be as prominent, get that much space.
Yea, after I saw your two new sections it made me realize that Assistants should just be a top-level menu item and then clicking that would list the individual assistants. Eventually it won’t make sense to show all the assistants in the left side, in the settings.
In terms of the display you suggest, I'll work on item 1,3.
Item 2: So you're saying instead of cards, just have in effect, a paragraph, per entry. Then an Edit link if applicable.
I was proposing to just have a row per entry and then either an “Edit” link on the row or maybe the whole row is just clickable.
Item 4: The "system" entries (have user_id=NULL) wouldn't be editable by any user, in my mind. So they wouldn't get shown in a form.
Oh, yes. Good point. I guess I would propose then: when you click on a system entry, it’s a show page (no form, no edit link anywhere). And when you click on a user one then it takes you to the form, and clicking save on the form just refreshes and keeps you on the form. I don’t think we need to toggle back & forth between an edit & show for user ones.
The one issue I’m aware of with this approach is that the top-right corner toast notification which says “Saved” is not quite prominent enough. I’ve observed people click save and not realize it saved. I plan to move that to the center of the screen and have it slide in a more visually prominent way.
| @@ -0,0 +1,5 @@ | |||
| class AddAPIServiceIdToAssistants < ActiveRecord::Migration[7.1] | |||
| def change | |||
| add_column :assistants, :api_service_id, :bigint, null: true | |||
There was a problem hiding this comment.
We should do this as an add_reference so it knows it’s a foreign key. That adds the DB constraint.
|
@stephan-buckmaster Recent changes of mine created some new conflicts on this branch. Let me know if you want help resolving those; I can take a pass at it. I was just about to do it but I stopped myself b/c I don't want to push a commit to your branch and risk colliding with you. Just let me know if you want help with those conflicts. |
|
The merge conflicts look managable. I'll let you know, @krschacht |
|
I believe I got through all your suggestions @krschacht. Still no tests (there's a few harmless failures). |
|
@stephan-buckmaster I did a full pass through the PR today and I think it's ready to merge in. I made some changes locally as I was reviewing and just pushed up a big commit ("Final pass changes") that blends them all. Here are my notes, just in case you're curious:
Oh, and you had a driver called "Built On" on the API Services, what was that? I think the user should have to select openai or anthropic, right? I removed it but I can revert if I'm mistaken. I really think it's ready for me to click "merge", but just since I've been staring at this for a few hours I'll do a user-facing run-through one more time tomorrow before and then click merge in the morning. I don't like to merge into main before calling it a night. :) Oh, the one thing I haven't done is actually create a Grok account and try adding Llama3 myself! I'll give that a try tomorrow. |
krschacht
changed the title
|
@stephan-buckmaster And it's merged! Whew. This project was quite a big lift. Today I tried adding Llama 3 myself, through Groq, and fixed a couple small bugs with that. It works so well that I went ahead and made Llama3 a new default within the app and pre-created Groq for people (alongside OpenAI and Anthropic). A few loose ends that I want to capture while it's top of mind:
|
|
@stephan-buckmaster You might appreciate hearing this: after upgrading my personal server to this latest PR, my APIs stopped working. I checked the API keys and they were copied over incorrectly. I've been digging in for the last couple hours trying to figure out what happened. I realized quickly that this is the raw database value in it's encrypted form (user has encrypts :openai_key) and I got stuck thinking this was a migration issue, like maybe DB encryption wasn't enabled in migrations because of a rails initialization issue. Finally, I realized that we removed Lesson learned! I don't have a ton of experience thinking about encrypted database contents so this was a new one to me. One reflection I've had on this whole PR is that it probably would have been worth us merging it in as several PRs along the way. However, the challenge with that is that all of the pieces were really tightly coupled together and the whole architecture was a bit in flux at each stage of the game. Maybe once we had really settled on the scope and had a big PR with lots of work, it probably would have been worth breaking off pieces at a time and landing them. One PR could have just added the models and populated the data, without all the views and the integration with AI backend. Then another one could update the views. Etc. Not a big deal, we got it done either way, but just an interesting reflection. |
About the encryption -- that is quite something, I tried it out, and found now problem, and then deleted the "encrypts" on User. The feature seemed easier to begin with, then became larger. It might be good to plan features in more detail prior to implemenation. One nice thing to add here would be a "check connection" button for API Services, because that can be difficult to know from the outside. |
|
Great! I handled the migration cleanup. I need to think more about how to help people upgrade/add new LLM models. I would like it to be a little more automated than that since not everyone keeps on top of every AI's companies latest announcements like we probably do. :) I've been thinking about some other mechanism within the app to announce new features so maybe it's related to that. I may setup some managed endpoint that I run, which every instance of the open source app can hit in order to get "announcements" which get displayed in some small, clean way in the UI. I'll keep thinking. Good point about the "check connection" button. I went ahead and created tasks for all of these things so I can keep track things: I'm probably going to tackle function/tool calling next since I've been actively working in that area anyway. If you want to take on any of these, just let me know! I know you're traveling and have contributed a lot so no pressure, I just wanted to keep you in the loop. |
…Bot#389) Co-authored-by: Keith Schacht <[email protected]>
This PR builds on #334 which introduced the ability to add new Assistants
This continues #385 (closed) My last comment there, #385 (comment) (@krschacht)
This PR: