Create new & manage Assistants

[stephan-buckmaster]

Want to get some feedback where to take this. No tests included.

When adding an assistant, it is tied to a model ... But I didn't want to hard-code the entries in the drop-down. So it seemed like a good time to store the "models" in a table. Can't really call a model just, model. So I added an ai_api_models table.

To reach the new feature with this change, go to "Settings," Then there will be a "New Assistant" link on the left. Save/Cancel work. Editing existing work too.

After one has added a nice collection of Assistants, this is not going to work anymore. The column will become too long, both on the settings pages, and on the regular pages. I suggest to do a "top 5", after which to show a link "more", which will open up a new page with a table of the assistants, which can be paged as needed.

Also haven't looked into the delete part

(To fix #308

[krschacht]

@stephan-buckmaster thanks for taking this on! I did a super quick skim on my phone and I think you’re on the right track.

The new table/model name is long. How about just LLM? You’d need to add that to the special rails config file that lets you adjust capitalization, inflections is maybe what it’s called?. We already did that once for “AI” so you can search. But LLM feels like the right concept since an Assistant subsumes it as a higher level concept. Plus, we may eventually have a table for image generation models.

I couldn’t tell if Assistant now has a foreign key to LLM table, but let’s do that so thinks are officially linked up. We should be able to do assistant.llm and llm.assistants for the basic two way association.

I forget the other notes on the issue but this is definitely looking good!

[krschacht]

I just re-read your notes and had one more thought: regarding the problems of too many assistants in the left nav, I think a decent approach for v1 would be:

• Let the page loading select all assistants
• If there are more than 5 then they are hidden via CSS and a “See more” link is present
• Clicking “see more” simply unhides the rest of them and they appear inline, pushing the conversations further down

It’s not the perfect long-term solution, but that’ll hold us for awhile. It saves having to create a new page or do any kind of pagination or anything like that. Plus, all that has to be solved for conversations long before it’ll be a problem for assistants. :)

[stephan-buckmaster]

The issue of https://github.com/allyourbot/hostedgpt/issues/335 would suggest a different UI: One would choose the system prompt together with entering the present question. Might be better. Of the schema of the assistants table only the name, model and instructions are of interest, at least to me. So 335 may be a better solution, and makes this one obsolete. Unless there are other kinds of configurations planned

[krschacht]

We need this anyway. I think 335 is solving a more narrow problem.

Someone recently asked if they could create an assistant for a different GPT-4 model (there are many variations) rather than the one we were defaulting too. also, soon we’ll support a bunch more LLMs and we don’t want to precreate an assistant for every one: we’ll want to let users manage which assistants they want.

Also, if you’ve ever played with the “create a GPT” functionality of ChatGPT — this PR you’re doing here lays the foundation for all that. When you create an assistant we’ll want to let you attach files to it so that it has specialized RAG context and we’ll want you to indicate which “tools” it has access to. So this is definitely important ground to lay.

[stephan-buckmaster]

Changes made as discussed. It seems the collapsed section is not always open when editing one of those assistants, is tailwind being flaky?

Can you confirm the list of initial records in the llms table, https://github.com/stephan-buckmaster/hostedgpt/blob/308-manage-assistants/db/migrate/20240507165222_create_llms.rb (@krschacht) ?

Still need to add tests, and add the soft-delete function. Please let me know of other items I may have forgotten

[krschacht]

@stephan-buckmaster I looked through things and left some feedback. Great work on all this! It's going to be a really solid addition.

I wasn't able to repro the collapse flakiness, but I had an overall suggestion on how to simplify it which should address that specific issue.

[stephan-buckmaster]

Renamed LLM to LanguageModel, and put the "best" logic there. It really should be linked by a language_model_id column in the assistants table. I would assume if someone had added manual model values (outside of this code) to the assistants table, records for these should become created by the migration in the language_models table, and then linked with the new id?
I tried to recreate the list of OpenAI / Anthropic models as suggested, it becomes a bit confusing to check I admit. Feel free to let me know if there's something to change.

[krschacht]

@stephan-buckmaster I finished going through this in detail. It's really close! A couple things:

• As I was reviewing the PR, I did some cleanup along the way to save us a little back-and-forth. I tried to make my commits atomic and self-explanatory so you're welcome to look back through them if you'd like, and I'm happy to answer any questions about them. To summarize: I tightened the visuals a bit, made small adjustments to logic to simplify, and the most notable thing I did was that I decided to remove the hiding >5 assistants from the settings page. Instead, we'll just list them all and I moved the "Your Account" above the list in case it gets really long. It was complicating the logic in that settings menu quite a bit, I stared at it and couldn't figure out how to simplify, and it's really most important for the user-facing side.
• There was a little bit of code that I almost removed but I wasn't certain it was okay to remove it, so I left inline comments about those. Take a look.
• One notable bit of logic we need to add: all newly created assistants have a boolean for images. This is mistakenly defaulting to false. Instead, we need to add a boolean column to our LanguageModels table such as supports_images. Then in assistant.rb we can have a before_save which sets the self.images by referencing the language_model

After these items, it's ready for any updates to tests. To make your life easier :) I think you can skip adding any new system tests, you can just fix up any new existing. The visual changes are minimal. But I do think it's worth us adding model & controller tests for all the various new checks & states.

[stephan-buckmaster]

@krschacht, you wrote, "so I left inline comments about those". I think I got them all above, please let me know if I overlooked any. Just pushed small updates. I'll look into the image thing now, then tests.

When I merged in the new changes in the main branch, I couldn't submit any messages anymore, so that would be another hurdle, after

[krschacht]

I had temporarily broken main. There is a fix in there now so pull again.

[krschacht]

@stephan-buckmaster You mentioned you got all my inline comments, but I'm not seeing any comments on those from you or any changes to the code. My comments are in the history stream above ^. Maybe you haven't pushed your changes yet because of broken main?

[stephan-buckmaster]

Ok just pushed changes, hope to address all the review comments, including adding a support-images column for the language-models table. With respect to which: I don't quite understand why assistants would not assume the support-image setting from their language-model. (I would add a validation that an assistant's language-model can only be change when image-support is the same, or doesn't matter because no images have been used). Going to be adding tests ...

[krschacht]

@stephan-buckmaster Oh jeez, you're right about the supports images column! I spoke too quickly when I made my suggestion. You're absolutely right that we don't need it at all in assistant.rb any longer so you can drop it from there; we'll just always reference the language_model.supports_images

I looked quickly at your changes and all looks good! I'm going to think about this version_navigate_message_params a little further, it's kind of a hack that I put in place there which you're working around. But I think it's a fine solution for us to run with.

[stephan-buckmaster]

Ok @krschacht , pushed first round of test changes, I think there are some tricky display items left, from the view changes. More tomorrow.

What did you think about validating changing languge model for an assistant with respect to keeping the "supports_images". It might not matter to go from supports to doesn't-support.

[krschacht]

@stephan-buckmaster Hmm, we'd need to make sure it doesn't throw an exception. Meaning, if you had a conversation with an assistant that was using GPT-4 and you added some images. Then you switch that assistant to be GPT-3.5 to save money and you go back to that same conversation, you would see images in the conversation history. But when you try continuing the conversation, it should handle that gracefull.

I believe it will — I think within backend/openai there is a method such as preceding_messages which constructs the message array to send over, and that should consider whether each individual message has an image AND if the assistant's language model supports images.

[stephan-buckmaster]

I just tried attaching a simple text file and things fell apart with a 500 error. So I feel this is not the issue to sort out how to deal with images.

[krschacht]

Only images are supported right now. Someone recently reported a bug that the file selector is not configured to filter only for images so it may have let you attach non-images but that will definitely error out. Try again with an image and it should work.

But I do agree this is really an edge case and not something we need to concern ourselves with much right now.

On Wed, May 22, 2024 at 10:27 PM Stephan Wehner @.***> wrote:

I just tried attaching a simple text file and things fell apart with a 500 error. So I feel this is not the issue to sort out how to deal with images.

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

[stephan-buckmaster]

Just a headsup, some disruptive changes came in on main. Trying to figure them out

[krschacht]

@stephan-buckmaster oh shoot, yes, this is from a PR I just merged in. I hope you don’t have to spend much time on this — if there is anything that looks tricky I can look at this first thing tomorrow and help in resolving any conflicts. I wrote all this code so I have all the context fully loaded.

I believe the only file that had significant changes in this bunch is ai_backends/open_ai.rb

[stephan-buckmaster]

Sorry, I tried ... busy tomorrow. So revisit on Saturday

[krschacht]

@stephan-buckmaster I merged main into your branch and resolved all the issues. There was really just one tricky thing:

Previously, the OpenAI backend was not actually stubbing out streaming responses. I thought that was unnecessary when I initially wrote it. However, when I implemented OpenAI tool calling, there was a lot of trickiness related to streaming so I improved the fidelity of how that's stubbed out. That was in another PR which landed in main ahead of yours.

So... when I was merging main in, I saw that you also re-worked some of how stubbing out OpenAI works because you wanted to confirm that the model was flowing through properly. This was a good thought, but our two additions were a little tricky in working together. I managed to figure out a solution. It's contained within this single commit: https://github.com/AllYourBot/hostedgpt/pull/334/commits/b80b060007540d6903a309a2e95400cffbd89aae

I'll try to summarize it in words:

Whenever you want to use the OpenAI test backend, you need to mock TestClient::OpenAI.api_response and set it either equal to TestClient::OpenAI.api_text_response -or- TestClient::OpenAI.api_function_response. In addition, you should then mock the corresponding .text or .function with what you want the response to be.

In the event you want the response to have a text response which dynamically includes the model name, you can set the .text to nil and it'll fall back to a special text string. Whew.

Anyway! I hope this sets you up to land the PR. Let me know where things stand and if there's anything else you need help with.

[krschacht]

Oops, one small bug in that special commit. I just changed the default text to:

  def self.default_text
    "Hello this is model #{@@model} with instruction nil! How can I assist you today?"
  end

I had accidentally hard-coded the model name! I pushed one more commit.

[stephan-buckmaster]

@krschacht , this went well! I expanded the pull request this morning with one commit.

Then I worked on a change to validate that "downgrading" the language model for an assistant will not be problematic, by looking into whether any messages of an assistant have documents (which for now are meant to be images).

However, I find that the has_many :documents defined in the Assistant model does not cut it. When I start a new conversation (at commit 1982578 of upstream-main) and upload an image with my first question, I find the documents record created has an empty assistant_id. This is logged:

INSERT INTO "documents" ("user_id", "assistant_id", "message_id", "filename", "purpose", "bytes", "created_at", "updated_at") VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING "id"^[[0m  [["user_id", 1], ["assistant_id", nil], ["message_id", 1], ["filename", "IMG_####.jpg"], ["purpose", "assistants"], ["bytes", 193920], ["created_at", "2024-05-26 01:09:34.873474"], ["updated_at", "2024-05-26 01:09:34.873474"]]

So I have a commit which I haven't pushed, which adds this to the Assistant model:

has_many :message_documents, class_name: 'Document', through: :messages, source: :documents, dependent: :destroy 

Then assistant.message_documents.exists? will reflect whether there are any conversations with images added for that assistant. And I can do my validation based on that; also only display the eligible LLM's in the drop-down

I could also add code to

1. update the assistant column in the documents table according to the assistant_id in the messages table in the migration, and
2. make sure that the image uploads actually get the documents.assistant_id populated.

Do you have an opinion / advice? Sorry this is all getting more involved than it first seemed

[krschacht]

@stephan-buckmaster that sounds like a good solution. The fact that the assistant_id isn’t being populated is a miss that is probably worth fixing.

The thing that confuses me a little bit still is that I expected this line to catch it: https://github.com/AllYourBot/hostedgpt/blob/1982578bea4c0c361d17ddbeadb2da6385456d1b/app/services/ai_backend/open_ai.rb#L127

One thing I’ve been considering for awhile is that there may be some day be multiple assistants involved in a conversation. Even though that isn’t true yet, I did already introduce the ability for you to change an assistant mid-stream of a conversation. I don’t know if you’ve seen this feature, but if you click the re-generate button beneath any image it gives you the option to re-generate with a different assistant.

At the time a new message is being generated, the assistant is passed into the ai_backend/open_ai.rb method. When we hit the API, we pass all the previous messages in the conversation along in the API call so we have to quickly reconstruct those messages. That’s what this preceding_messages method is that I linked to above, and you see it has a check where on-the-fly it confirms if the assistant you’re chatting with supports images or not.

I would have expected downgrading an assistant to “just work” because of this check. If it doesn’t, there must be something more subtle that I’m not understanding.

In any case, I think your proposed fix sounds like a good one so feel free to run with that.

[stephan-buckmaster]

That line prevents feeding images to an assistant/model that can't process them. So things don't fall apart and a response will be generated. But you would expect a less meaningful response if the assistant's model was changed in the meantime.

So in my mind, if a user changes the model of an assistant, and they have used it for processing images, they would want that to stay . If they do regenerate after a model-change -- and it doesn't include the image, even when it is visible on the screen, wouldn't that be surprising, even for understanding the response?

[krschacht]

I think that surprise would be an acceptable one. It’s not a huge deal. I’ve tried it multiple times when changing from gpt4 to 3.5 and it works surprisingly well bc the model will just BS based on the conversation text history, but it won’t let you attach any new images in the composer.

There is also the question of: what happens if you downgrade models but then later re-upgrade. I used to do this a lot because of rate limits.

So in my mind, the fact that it doesn’t blow up is a good bare minimum. And I’m not sure what the right “best” solution would be.

You caught that we are missing an association, which is worth fixing regardless. But I don’t think we want to delete or even hide images, because you may still want to reference those or switch back to a new image-capable model.

[stephan-buckmaster]

Yes, when regenerating responses with an assistant that doesn't support images, it is pretty transparent, the popup really works. When I use an assistant that doesn't support images it has responded with

I tried to retrieve some information to help answer your question, but I encountered an unexpected error. Could you provide more details regarding the image you mentioned as "ABC"?

So I dropped the validation on model/assistants. But added migration to populate the assistant-id for documents if possible.

I looked through the exchange above, and I think I covered all feedback. (Plan to continue with the 341 issue)

[krschacht]

@stephan-buckmaster great, thanks! so just be sure: this PR should be ready for me to give one final pass and then merge in?

[stephan-buckmaster]

Yes; hope all is covered.

[krschacht]

@stephan-buckmaster can you explain the "allow_deleted_assistant" flag? What is the case where we need to still support updating messages which are part of an assistant which has been deleted?

[stephan-buckmaster]

This is the versions navigation thing. Without the flag, when it is commented out,

 errors.add(:assistant, 'has been deleted') if assistant.deleted? ######## && !allow_deleted_assistant`

this test fails:

MessagesControllerTest#test_even_when_assistant_is_deleted_update_succeeds_for_a_new_version_in_the_URL

[krschacht]

@stephan-buckmaster Oh shoot... as I'm going through the code in detail, it just occurred to me why some documents are not associated with assistants.

Documents can either belong to messages -or- assistants, according to the OpenAI domain model. https://platform.openai.com/docs/api-reference/files (I had to rename "File" to "Document" since file is a reserved word in Ruby).

I set up these models to reflect the domain model of OpenAI. However, we aren't fully utilizing this domain model yet since I haven't implemented the Assistants API. I think I need to unwind a little bit of this.

I can handle this on the branch, it's a small change. However, I didn't fully understand what problem it was causing when you discovered that assistant_id was nil on some documents? I too thought it was a bug, when originally discovered, but did you wire that up just because you caught the oversight or because there was something odd that was happening by having those not connected?

[stephan-buckmaster]

Interesting. If the message has an assistant, then the document belongs to a message doesn't really need an assistant column. So it looked redundant to me, so I assumed it was for optimization. But if it is not always populated, then it looked like a gap.

Are you saying an assistant can respond, and include documents which will then be tied to the message? Again, the message's role (being assistant) would tell the same thing then, making the assistant-id column in documents redundant.

So yes, if it is not helpful, might as well remove commits e49eef9 and ea07426. It almost seems like the test assertions should be negated

[krschacht]

In the OpenAI universe, you can create an Assistant (they call it a GPT). You can configure this with custom instructions, give it access to tools, and optionally give it access to files/documents. When a document is associated with an assistant, than any conversation you have with that assistant, it may choose to reference the document when formulating a reply. Whereas when a document is associated with a message, then it's only in the context of that message history (i.e. conversation) that the assistant would reference the document. Again, HostedGPT doesn't have this capability built yet to reference assistant's documents but when I was creating those models and reading through the API docs I thought: "that makes sense! we should support that too." :)

I went ahead and reverted a couple of the small assertions and pushed a new commit. I'll continue working my way through the PR, and cleaning up a couple things here and there, and ultimately merge it into main soon (probably not today but hopefully tomorrow). But lots in those two commits of yours is still good code, like creating a Message with an associated Document — I never had that test case in there, and I should have. But I just flip the bit on a couple assertions around document.assistant.

Onward! This PR has tons of good stuff and I'm excited to get it in so I can continue building upon it, since I'm doing so much with tools and I want to start letting people give different tools to different assistantas.

[krschacht]

@stephan-buckmaster in this PR you've added ENV variables for the development & test database names. However, rails has a built-in way to override the database: DATABASE_URL.

The format is: postgres://username:password@localhost:5432/custom_database_name"

Local often has no username & password and default port so so it looks like this (note the three slashes) DATABASE_URL="postgres:///custom_database_name"

Of course, if you are overriding for development and test you'd want to do this in each environment:

DATABASE_URL="postgres:///development_database_name" bin/rails server
DATABASE_URL="postgres:///test_database_name" bin/rails test

Would this work for you or do you think you still need a custom ENV variable for your use case? I'd probably put my dev one into my bash profile so that it's always present in my shell and then have a bash alias for running tests which does that whole test command with a different database_url

[stephan-buckmaster]

It would be a 80% solution to just use DATABASE_URL (usually I have the git branch logic inside database.yml)

I don't quite understand your reluctance, to me it looks quite transparent. In terms of security there is alreay ENV evaluation in the file. What could go wrong? Someone who doesn't set the var will not notice any difference. And for prod there is no change.

[krschacht]

I’m good with it! I’ll leave it in. It sounds like DATABASE_URL doesn’t work for your context so happy to have the extra ENV.

[stephan-buckmaster]

Thanks for all the feedback above, @krschacht !