langchain-ai / opengpts

MIT License
6.44k stars 851 forks source link

Delete Assistants #321

Closed esxr closed 5 months ago

esxr commented 5 months ago

Added the feature to delete assistants

mkorpela commented 5 months ago

Linter failures can be fixed with make format on backend and yarn format on frontend.

mkorpela commented 5 months ago

assistant is most likely connected to threads. Those threads in my opinion should be updated to empty assistant when the assistant is removed.

sunilgovind commented 5 months ago

hi @esxr Thank you for your PR. Awesome. Please take a look at the issue https://github.com/langchain-ai/opengpts/issues/293 I added some of the details that we are looking for. Thank you if needed, let us incorporate those as well.

esxr commented 5 months ago

assistant is most likely connected to threads. Those threads in my opinion should be updated to empty assistant when the assistant is removed.

It makes sense, and without it, there is a bug at the moment which i noticed. Here’s how to simulate: •⁠ ⁠create an assistant •⁠ ⁠⁠have a conversation •⁠ ⁠⁠you’ll see a thread •⁠ ⁠⁠now delete the assistant •⁠ ⁠⁠assistant is deleted, but when you click on the thread, you can see nothing in it

Screenshot 2024-04-26 at 2 42 11 pm
esxr commented 5 months ago

There are two ways to look at this problem: 1.⁠ ⁠If an assistant is removed, we will remove all associated threads as well. This is simple to implement, however, it may be a little user unfriendly. 2.⁠ ⁠Alternately, and more properly, assistant removal should just indicate that the thread state is now frozen, and user cannot any longer continue that conversation. So, all interactions with the thread detail view should be disabled.

Which one of this should we go for @mkorpela?

bakar-io commented 5 months ago

This was discussed before and the decision was made to enable assigning a new assistant to a thread whose original assistant got deleted. So basically, whenever there is an attempt to open a thread which no longer has an associated assistant, prompt the user to choose a new assistant from existing assistants.

samuelp-mw commented 5 months ago

This was discussed before and the decision was made to enable assigning a new assistant to a thread whose original assistant got deleted. So basically, whenever there is an attempt to open a thread which no longer has an associated assistant, prompt the user to choose a new assistant from existing assistants.

Do we then imagine "nullifying" the assistant ID for all the threads associated with the assistant? This way it is possible to easily identify all the dangling threads directly.

bakar-io commented 5 months ago

Do we then imagine "nullifying" the assistant ID for all the threads associated with the assistant? This way it is possible to easily identify all the dangling threads directly.

Exactly.

andrewnguonly commented 5 months ago

So basically, whenever there is an attempt to open a thread which no longer has an associated assistant, prompt the user to choose a new assistant from existing assistants.

@bakar-io was there any consideration for only allowing assignment of assistants that "support" the existing messages types from a thread?

For example, can/should a plain chatbot assistant be assigned to a thread that contains Tool messages?

esxr commented 5 months ago

This was discussed before and the decision was made to enable assigning a new assistant to a thread whose original assistant got deleted. So basically, whenever there is an attempt to open a thread which no longer has an associated assistant, prompt the user to choose a new assistant from existing assistants.

@bakar-io ok let me implement that

bakar-io commented 5 months ago

was there any consideration for only allowing assignment of assistants that "support" the existing messages types from a thread?

@andrewnguonly No. Details were not discussed - only the general approach.

bakar-io commented 5 months ago

@andrewnguonly I just tested and your intuition is correct: assigning a thread with tool_calls to chatbot leads to a deserialization error. This error occurs when an attempt is made to continue chatting with this newly moved thread.

To keep things simple and straightforward, I propose that when a bot is deleted, all of its threads are also automatically deleted.

Here's why I think that's the best approach:

Before a bot is deleted, each thread can check their parent bot's type (one of: chatbot, chat_retrieval, agent). But once a bot is deleted, this information is no longer accessible to a thread. The only way at this point would be to iterate through a thread's messages and detect if tool_calls is present. This seems to me a bit of a hack. So, I think that moving threads from bot A to bot B should be done before deleting bot A.

We can split this flow into two PRs. This PR can simply focus on deleting bot and deleting all associated threads. The second PR can enable users to attach threads to suitable bots. When a bot is deleted from the frontend, we can require user to confirm deletion and let them know that all associated threads are going to be deleted (and if they want to keep the threads, they should move them first).

andrewnguonly commented 5 months ago

To keep things simple and straightforward, I propose that when a bot is deleted, all of its threads are also automatically deleted.

To keep the scope of this PR small, I'm fine with this approach. However, I suggest including a warning or some kind of confirmation before deleting all associated threads as part of this initial change.

bakar-io commented 5 months ago

Hey everyone. Just a heads up - I've made a new PR because we really need to get this feature shipped ASAP and this PR was dragging a bit. @esxr your efforts are genuinely appreciated and I hope that next time we won't have a crazy time constraint like this.

esxr commented 5 months ago

@bakar-io thanks for this! Had to be out for a week for a personal engagement.

ptgoetz commented 5 months ago

Closing in favor of the alternate PR. Thanks @esxr and @bakar-io for your efforts!