Create new & manage Assistants #334
Conversation
…crypted_data setting and use update! instead of update in tests
…nfig/database.yml.sample
through ENV variables
|
@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! |
|
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:
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. :) |
|
The issue of #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 |
…t_id, through a db migration and in test fixtures
|
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
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) |
|
@stephan-buckmaster great, thanks! so just be sure: this PR should be ready for me to give one final pass and then merge in? |
|
Yes; hope all is covered. |
|
@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? |
|
This is the versions navigation thing. Without the flag, when it is commented out, this test fails: |
|
@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? |
|
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 |
|
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 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. |
|
@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) Of course, if you are overriding for development and test you'd want to do this in each environment: 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 |
|
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. |
|
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. |
|
Thanks for all the feedback above, @krschacht ! |
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