-
Notifications
You must be signed in to change notification settings - Fork 202
feat: add support for delete_all_documents for astra client
#2362
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
Conversation
a8f1f67 to
823b2ad
Compare
823b2ad to
81c288b
Compare
mpangrazzi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work! I left some comments.
In addition, I see there is no error handling logic here, i.e. no try / catch login if delete_all_documents fails. Please have a look at other implementation and update tests / PR.
| if deletion_counter == -1: | ||
| logger.info("All documents deleted") | ||
| else: | ||
| logger.info("Could not delete all documents") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logging logic can be improved. What about removing this in favour of something like:
logger.info(f"{deletion_counter} documents deleted")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpangrazzi The reason for this if-else was -1 which is the response provided by Astra client for successful delete all documents. But in general for a user mindset -1 would mean a error code. Hence I wanted to give a message instead of a counter value returned by Astra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srini047 Got it! So in this case I would do a better error handling: logging it with proper level (i.e. error), get the message (is it available somewhere?). WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpangrazzi This is the response from the Astra API. There's something called raw_result attribute but don't think it has a definite string/msg that we can parse and notify the user with actual error.
We should be handling it from our end but anyways don't have enough data to share with users. So I think this should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just changed the verbosity level.
integrations/astra/src/haystack_integrations/document_stores/astra/document_store.py
Outdated
Show resolved
Hide resolved
6ecc890 to
a27a50c
Compare
a27a50c to
28e6fad
Compare
delete_all_documents for astra client
mpangrazzi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Related Issues
delete_all_documents()operation toAstraDocumentStore#2308Proposed Changes:
delete_all_documents()in document_stort and clientdelete_allfrom thedelete()to decoupleHow did you test it?
cd integrations/astra hatch run test:integrationdelete_all_documents()wheredelete(delete_all=True)was used earlierNotes for the reviewer
A quick question: This is a breaking change do we still need the backward compatibility?
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.