search

AllYourBot / hostedgpt

An open version of ChatGPT you can host anywhere or run locally.
MIT License
304 stars 122 forks source link

Create & manage LanguageModels and APIService (+add Llama 3) #389

Closed stephan-buckmaster closed 1 month ago

stephan-buckmaster commented 1 month ago

This PR builds on #334 which introduced the ability to add new Assistants

This continues https://github.com/AllYourBot/hostedgpt/pull/385 (closed) My last comment there, https://github.com/AllYourBot/hostedgpt/pull/385#issuecomment-2150042388 (@krschacht)

This PR:

krschacht commented 1 month ago

@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.

stephan-buckmaster commented 1 month ago

The merge conflicts look managable. I'll let you know, @krschacht

stephan-buckmaster commented 1 month ago

I believe I got through all your suggestions @krschacht.

Still no tests (there's a few harmless failures). Let me know if you see anything off, while I work on those ...

stephan-buckmaster commented 1 month ago

Ok, pushed rather large changes, including many tests. Could have some gaps, I'll revisit on the weekend.

Let me know your thoughts in the mean time.

krschacht commented 1 month ago

Great, I'll give it a close read through. Are you okay if I commit some changes to this branch as I'm reading through it? If I notice little clean up things it's often faster for me to just make the change right then, but I don't want us to step on each others' toes. :)

stephan-buckmaster commented 1 month ago

Sure go ahead. (I realized I didn't work on the display of system-wide language-models, they look a bit weak.)

stephan-buckmaster commented 1 month ago

I hope my comments are visible above , many "Done". Let me know about the test changes, please @krschacht . Still need to look at the delete/destroy issue you mentioned

krschacht commented 1 month ago

I hope my comments are visible above , many "Done". Let me know about the test changes, please @krschacht . Still need to look at the delete/destroy issue you mentioned

Ah yes, I forgot I had not reviewed tests. I just did that now and they look good. Nice job with the test coverage. There’s a fine line between not enough testing and too much and I think you nailed it. My one comment is around keeping the fixtures “real data” for the two models we’re going to populate in production (language_model and api_service).

stephan-buckmaster commented 1 month ago

Ok got through more feedback. Now what I see outstanding is to create all records on the user level, including APIService records for OpenAI/Anthropic with their API URL's.

krschacht commented 1 month ago

I’m not sure we need a gem for this. But I’m not sure what symptom we’re trying to address with this. This was in the other PR and I managed to eliminate it. But I may be missing something.

On Sun, Jun 16, 2024 at 12:48 PM Stephan Wehner @.***> wrote:

@.**** commented on this pull request.

In app/models/user.rb https://github.com/AllYourBot/hostedgpt/pull/389#discussion_r1641930419:

  • def usable_language_models
  • LanguageModel.for_user(self).not_deleted
  • end
  • def destroy_in_progress?
  • @destroy_in_progress
  • end
  • def destroy
  • @destroy_in_progress = true
  • begin
  • super
  • ensure
  • @destroy_in_progress = false
  • end
  • end

Ok made change, there's probably a gem for this. Look one up?

— Reply to this email directly, view it on GitHub https://github.com/AllYourBot/hostedgpt/pull/389#discussion_r1641930419, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIR5P2L3TKGVYWXYINQXLZHXFWLAVCNFSM6AAAAABI3HLV7GVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMRRGUZDIMBQGI . You are receiving this because you were mentioned.Message ID: @.***>

krschacht commented 1 month ago

I think that’s right. That and the questions on that destroy logic, I’m still not clear if we need any of that.

On Sun, Jun 16, 2024 at 12:49 PM Stephan Wehner @.***> wrote:

Ok got through more feedback. Now what I see outstanding is to create all records on the user level, including APIService records for OpenAI/Anthropic with their API URL's.

— Reply to this email directly, view it on GitHub https://github.com/AllYourBot/hostedgpt/pull/389#issuecomment-2171786399, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIR5OKQRCV2VT453QXLSLZHXF33AVCNFSM6AAAAABI3HLV7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZRG44DMMZZHE . You are receiving this because you were mentioned.Message ID: @.***>

krschacht commented 1 month ago

Oh, sorry, earlier I was commenting from my phone and I missed you had already done the delete! thing. That looks good! So yes, it’s just this APIService’s change and then we can merge in!

stephan-buckmaster commented 1 month ago

Am I going too far? If all Language Models are assigned to the user, and all users will have copy of the standard API Services, then these APIService records should have the openai-key / anthropic-key. So these two keys should be copied from the users records to the new api_services record -- and the columns should be removed from users table. I think people will get confused if there's so many different places for configuring.

How does this fit in with the sign-up sequence?

krschacht commented 1 month ago

@stephan-buckmaster dang, you’re right. This won’t hurt the signup sequence. Currently signup completely ignores the API keys and when you try to chat with an assistant the very first time then it responds with some hard-coded text instructions. Those will be easy to tweak.

stephan-buckmaster commented 1 month ago

All right, so I'll work out this reorganization. At least existing users should see no difference.

krschacht commented 1 month ago

👍 should we merge in this PR and do that one as a new one? That would let me do a little clean-up of the views in parallel, by adding this new view component pattern I'm thinking of. But if you think it's easier to keep this one open and add to this, I'm okay with that too.

I'm conscious that I do keep causing small merge conflicts with this PR as I merge other work into main.

stephan-buckmaster commented 1 month ago

I may have it done in a few hours. But if you don't see a commit -- feel free to merge.

(Update -- would take longer, so leaving merge up to you)

krschacht commented 1 month ago

I got a bit bogged down on my PR so I haven’t gotten a chance to circle back on this yet.

stephan-buckmaster commented 1 month ago

I have pushed what I have now except for moving to user-managed language model records only. That's because it really is a larger change, it may not be quick review. I made a third pull-request for that, https://github.com/AllYourBot/hostedgpt/pull/425

krschacht commented 1 month ago

@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 commented 1 month ago

@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:

  1. I'm going to do some more UI work on the settings section of the app
  2. When creating a new language model, you had a driver called "built on". Do we need this option? I removed it but I want to confirm that I didn't break something.
  3. As I was playing with Groq I realized that the LanguageModel model should probably also have token_limit (for input) and supports_tools (boolean) since the AI backend should also consider these properties
  4. You left a comment in another thread about adding new APIs via migrations. I agree this feels quite hacky. Do you have any better ideas? I just did it again with the Llama 3 addition.
  5. Within this PR we just merged in I went ahead and left the self.create_without_validation method in LanguageModel. I saw you fixed up a couple bugs yesterday after my big commit so thank you for that. Could we move it back into the migration? I didn't quite understand why it broke when I tried to move it back there.
krschacht commented 1 month ago

@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. user.openai_key got copied over as {"p":"Qhq8i6vTyEoEUzqwmcbBQkj6ut/LUvZhdPbIQvR7wcmYq0cMKIshZFN4ufHwSR9KxntMnMBNbYrPGUXxRMU8IZb8iinEH6lhBBN7r0y46kmCDfktc6uc1sE+GLSjBkf6cfSgUJAJz6v+2K9u","h":{"iv":"AOUUAmbvsuDZMUFJ","at":"gHCXP8+k0tpyaoRuPWtzKA=="}}

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 encrypts :openai_key from the User table in this same PR which does the migration to copy that value over! This was the fix I just committed:

https://github.com/AllYourBot/hostedgpt/commit/4764a7b5a733fbdeaa216c507489579b0c466ca5

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.

stephan-buckmaster commented 1 month ago
  1. Ok
  2. About "Built-in." That is obsolete because we assigned them directly now
  3. Ok, not something I am familiar with
  4. I feel users can configure their own, just need to know what to do when a new model comes out. They have so many options now. E.g. They can have "OpenAI cheap" "OpenAI premium" .... then when prices change assign what works.
  5. Sure, it can go back, it is used in two migrations though, so doesn't look good

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.

krschacht commented 4 weeks ago

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: https://github.com/AllYourBot/hostedgpt/issues?q=sort%3Aupdated-desc+is%3Aopen+label%3A%22LLM+foundation%22

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.