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

Add "Delete" button for users to be able to delete files in copilot-chat-app sample #89

Closed
wants to merge 12 commits into from

Conversation

kenzabenjelloun
Copy link

@kenzabenjelloun kenzabenjelloun commented Aug 2, 2023

Hello !

I was trying to add a "delete" button in the copilot-chat-app sample to be able to delete files, it seems to work but I don't know if I'm doing it right. I am still a beginner in building webapps, so I was wondering if I could get some help here. (I don't know if a pull request is the best way to do it ? but I thought it might be a nice feature to add to the sample). I made a couple of changes in DocumentsTab and useFile and added DocumentDeleteService and DocumentDeleteController, if someone can help me pinpoint where I might be doing something wrong, I would be really grateful.

Thank you !

@github-actions github-actions bot added webapp Pull requests that update Typescript code webapi Pull requests that update .net code PR: ready for review labels Aug 2, 2023
@kenzabenjelloun kenzabenjelloun changed the title add delete button Add "Delete" button for users to be able to delete files in copilot-chat-app sample Aug 2, 2023
webapi/appsettings.json Outdated Show resolved Hide resolved
@glahaye
Copy link
Collaborator

glahaye commented Aug 2, 2023

@teresaqhoang @TaoChenOSU I think you may be working on something similar already?

webapi/appsettings.json Outdated Show resolved Hide resolved
@TaoChenOSU
Copy link
Collaborator

Hello @kenzabenjelloun,

Thank you for your PR!

This is a very nice feature to add, and we are happy that you are considering it.

Document handling is an essential part of the chat copilot app. Each imported document will generate data in 3 places:

  1. The memory source repository
  2. The chat message repository
  3. The memory storage (in the form of embeddings)

To completely remove a document, we need to remove the date generated in all these 3 places. This PR only removes the memory sources. We encourage you to add implementation to complete the deletion of documents.

P.s. To delete a resource, you should use the delete request: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/DELETE.

Copy link
Contributor

@crickman crickman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the document still present in vector database (Document Memory)?

@gitri-ms
Copy link
Collaborator

Hi @kenzabenjelloun - This is a great start! I've added some additional suggestions on top of what my colleagues said earlier.

Are you still interested in pursuing this PR? I just want to check since I haven't seen any updates from you in a couple weeks.

  • If yes - I recommend you review the suggestions and take a look at Tao's comment about how to remove all the document's data
  • If no - no worries! If I don't hear from you within the next couple days, I will go ahead and close this PR. If you want to come back and work on it at a later date, you are welcome to reactivate it.

@gitri-ms gitri-ms added enhancement New feature or request help wanted Extra attention is needed labels Aug 23, 2023
@kenzabenjelloun
Copy link
Author

Hi @gitri-ms ! Yes, I would still like to work on it. Can I just get some pointers on when the embeddings are generated in the code ? I'm having a bit of trouble figuring it out (I thought it would be directly after the document is imported but I don't see it in DocumentImportController?)

@crickman
Copy link
Contributor

FYI - DocumentImportController is being deleted in this change: #152

At this stage, introducing the feature might be best accomplished after #152 is merged.

Note: Your change and this update may overlap: #193

@gitri-ms
Copy link
Collaborator

gitri-ms commented Sep 7, 2023

Hi @kenzabenjelloun - thank you again for this contribution. However, given the large refactor coming with #152, we are unable to accept this change at this time. For that reason, I am going to close this PR for now.

We may be interested in revisiting it with you once the semantic memory refactor code comes in. However, I do want to note that while we welcome community contributions, we have limited engineering resources to assist with implementation of features that aren't in our project roadmap.

Going forward, for new features like this, we would appreciate if you would first open an issue with us to discuss the feature you want to bring in and its design/prioritization.

Alternatively, you are always welcome to maintain a fork of Chat Copilot and build on it there! I also encourage you to join our Discord community to engage with other developers building on SK and Chat Copilot, as they might have ideas on how to go about implementing new features like this.

@gitri-ms gitri-ms closed this Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed webapi Pull requests that update .net code webapp Pull requests that update Typescript code
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants