Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete Assistants #321

Closed
wants to merge 7 commits into from
Closed

Delete Assistants #321

wants to merge 7 commits into from

Conversation

esxr
Copy link
Contributor

@esxr esxr commented Apr 25, 2024

Added the feature to delete assistants

@mkorpela
Copy link
Collaborator

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

@mkorpela
Copy link
Collaborator

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

@sunilgovind
Copy link

sunilgovind commented Apr 25, 2024

hi @esxr
Thank you for your PR. Awesome.
Please take a look at the issue #293
I added some of the details that we are looking for. Thank you
if needed, let us incorporate those as well.

@esxr
Copy link
Contributor Author

esxr commented Apr 26, 2024

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
Copy link
Contributor Author

esxr commented Apr 26, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

esxr commented Apr 27, 2024

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
Copy link
Contributor

bakar-io commented Apr 27, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

bakar-io commented May 3, 2024

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
Copy link
Contributor Author

esxr commented May 3, 2024

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

@ptgoetz
Copy link
Collaborator

ptgoetz commented May 3, 2024

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

@ptgoetz ptgoetz closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants