core[patch]: throw exception indexing code if deletion fails in vectorstore

[KeiichiHirobe]

The delete methods in the VectorStore and DocumentIndex interfaces return a status indicating the result. Therefore, we can assume that their implementations don't throw exceptions but instead return a result indicating whether the delete operations have failed. The current implementation doesn't check the returned value, so I modified it to throw an exception when the operation fails.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
langchain	⬜️ Ignored (Inspect)	Visit Preview		Dec 13, 2024 8:46pm

[Vruddhi18]

The changes made are valid.

[eyurtsev]

We'll need to do the following:

1. Improve the error message
2. Use a more specific exception (you can subclass from LangChainException).
3. If elif is untermedinated and needs to have a branch for the else that will raise a not implemented error.

---

It's not clear that desired behavior for most users is to raise an exception by default. I'd probably expect that most of the time, what users would want is an error logged via the python logger. In some cases, users may want to configure this to be stricter and raise an exception.

---

What exception did you bump into?

Very frequently things fail because:

1. Serer is just down (OK to raise for this)
2. Transient issue (e.g., network connectivity dropped or client issues too many requests) -- these types of errors need to be retried at the implementation vectorstore/document indexer implementation level

[KeiichiHirobe]

Thank you for your feedback.

I'd probably expect that most of the time, what users would want is an error logged via the python logger.

I know what you mean, but still, I believe we should throw an exception here.
Let's say users update documents, and the indexer library fails to delete records in a vector database and proceed to the next step without stopping, then deletes the records in the record manager. This may happen in the current logic. If it happens, the records in the vector database won't be cleaned up forever, right? Users will see stale data due to that. I think we should avoid this situation. If we throw it here, the next batch execution will fix the situation if the vector database has been recovered.
Actually, this might have already happend in our environment because the number of records in Qdrant was a little bit more than the number of records in the table of record manager for some reasons. I can't think of any other reason why such an inconsistency would occur. Of course, we always update records in the vector database and the record manager only through the indexer library. If you have any other possible reasons in mind, please let me know.
To fix this, we had to manually delete dangling records in the vector database.

Serer is just down (OK to raise for this)

I suppose this situation. I am not sure, but I guess cloud services for vector databases are less stable than traditional databases like RDS.

[eyurtsev]

OK, let's make it stricter for now!

If we were creating a more relaxed mode, we'd need to skip cleaning of the record manager if a vectorstore deletion fails (to avoid the situation you're describing). The information could still be surfaced via logger.error and potentially an additional field in IndexingResult.

Feel free to add if you think useful, but I'm OK with the stricter solution for now.

[eyurtsev]

Happy to merge if we can make the changes outlined above!

A unit test will be required as well for this to be merged.

[KeiichiHirobe]

@eyurtsev I submitted two additional commits. Could you check it?

[eyurtsev]

Here's the abstraction for vectorstores: https://github.com/langchain-ai/langchain/blob/master/libs/core/langchain_core/vectorstores/base.py#L124-L124

Vectorstore implementations should return False only when an exception occurs when trying to delete a record (e.g., can't reach database, bad query etc etc.)

If a record cannot be found but the operation is executed successfully by the server, the value from delete should be True (not False).

If users bump into issues due to this PR, we should double check that the vectorstore implemented the correct semantics.

[eyurtsev]

@KeiichiHirobe thank you for the contribution!

If you're using the indexing code, and are interested in contributing more... the one obvious thing that's missing is a delete API! A user may just want to delete data from the index (e.g., data that hasn't been updated in a while, or delete by source id etc.)

[KeiichiHirobe]

@eyurtsev

A user may just want to delete data from the index (e.g., data that hasn't been updated in a while, or delete by source id etc.)

Interesting. Actually, we implemented it partially in our code base like this:

    delta = timedelta(minutes=15)
    updated_before = datetime.now(timezone.utc) - delta
    num_deleted = 0
    uids_to_delete: list[str] = []
    while uids_to_delete := record_manager.list_keys(
        before=updated_before.timestamp(), limit=1000
    ):
        vector_store.delete(uids_to_delete)
        record_manager.delete_keys(uids_to_delete)
        num_deleted += len(uids_to_delete)
    logger.info(f"Deleted {num_deleted} stale docs updated before {updated_before}")

I think I can work on this in a few months.