-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix: defer KG rebuild during batch document deletion #2819
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3137,7 +3137,10 @@ async def aget_docs_by_ids( | |
| return found_statuses | ||
|
|
||
| async def adelete_by_doc_id( | ||
| self, doc_id: str, delete_llm_cache: bool = False | ||
| self, | ||
| doc_id: str, | ||
| delete_llm_cache: bool = False, | ||
| skip_rebuild: bool = False, | ||
| ) -> DeletionResult: | ||
| """Delete a document and all its related data, including chunks, graph elements. | ||
|
|
||
|
|
@@ -3170,6 +3173,10 @@ async def adelete_by_doc_id( | |
| doc_id (str): The unique identifier of the document to be deleted. | ||
| delete_llm_cache (bool): Whether to delete cached LLM extraction results | ||
| associated with the document. Defaults to False. | ||
| skip_rebuild (bool): When True, skip the per-document KG rebuild step. | ||
| The caller is responsible for performing a single deferred rebuild | ||
| using the entities/relationships returned in the DeletionResult. | ||
| Used by batch deletion to avoid N redundant rebuilds. Defaults to False. | ||
|
|
||
| Returns: | ||
| DeletionResult: An object containing the outcome of the deletion process. | ||
|
|
@@ -3178,6 +3185,8 @@ async def adelete_by_doc_id( | |
| - `message` (str): A summary of the operation's result. | ||
| - `status_code` (int): HTTP status code (e.g., 200, 404, 403, 500). | ||
| - `file_path` (str | None): The file path of the deleted document, if available. | ||
| - `entities_to_rebuild` (dict | None): Populated when skip_rebuild=True. | ||
| - `relationships_to_rebuild` (dict | None): Populated when skip_rebuild=True. | ||
| """ | ||
| # Get pipeline status shared data and lock for validation | ||
| pipeline_status = await get_namespace_data( | ||
|
|
@@ -3860,27 +3869,39 @@ async def adelete_by_doc_id( | |
| raise Exception(f"Failed to persist pre-rebuild changes: {e}") from e | ||
|
|
||
| # 8. Rebuild entities and relationships from remaining chunks | ||
| # When skip_rebuild is set (batch deletion), we hand the targets back | ||
| # to the caller so it can do one combined rebuild at the end. | ||
| if entities_to_rebuild or relationships_to_rebuild: | ||
| try: | ||
| deletion_stage = "rebuild_knowledge_graph" | ||
| await rebuild_knowledge_from_chunks( | ||
| entities_to_rebuild=entities_to_rebuild, | ||
| relationships_to_rebuild=relationships_to_rebuild, | ||
| knowledge_graph_inst=self.chunk_entity_relation_graph, | ||
| entities_vdb=self.entities_vdb, | ||
| relationships_vdb=self.relationships_vdb, | ||
| text_chunks_storage=self.text_chunks, | ||
| llm_response_cache=self.llm_response_cache, | ||
| global_config=asdict(self), | ||
| pipeline_status=pipeline_status, | ||
| pipeline_status_lock=pipeline_status_lock, | ||
| entity_chunks_storage=self.entity_chunks, | ||
| relation_chunks_storage=self.relation_chunks, | ||
| if skip_rebuild: | ||
| logger.info( | ||
| "Skipping per-doc rebuild (skip_rebuild=True), " | ||
| "%d entities / %d relations deferred", | ||
| len(entities_to_rebuild), | ||
| len(relationships_to_rebuild), | ||
| ) | ||
| else: | ||
| try: | ||
| deletion_stage = "rebuild_knowledge_graph" | ||
| await rebuild_knowledge_from_chunks( | ||
| entities_to_rebuild=entities_to_rebuild, | ||
| relationships_to_rebuild=relationships_to_rebuild, | ||
| knowledge_graph_inst=self.chunk_entity_relation_graph, | ||
| entities_vdb=self.entities_vdb, | ||
| relationships_vdb=self.relationships_vdb, | ||
| text_chunks_storage=self.text_chunks, | ||
| llm_response_cache=self.llm_response_cache, | ||
| global_config=asdict(self), | ||
| pipeline_status=pipeline_status, | ||
| pipeline_status_lock=pipeline_status_lock, | ||
| entity_chunks_storage=self.entity_chunks, | ||
| relation_chunks_storage=self.relation_chunks, | ||
| ) | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Failed to rebuild knowledge from chunks: {e}") | ||
| raise Exception(f"Failed to rebuild knowledge graph: {e}") from e | ||
| except Exception as e: | ||
| logger.error(f"Failed to rebuild knowledge from chunks: {e}") | ||
| raise Exception( | ||
| f"Failed to rebuild knowledge graph: {e}" | ||
| ) from e | ||
|
|
||
| # 9. Delete LLM cache while the document status still exists so a failure | ||
| # remains retryable via the same doc_id. | ||
|
|
@@ -3940,26 +3961,33 @@ async def adelete_by_doc_id( | |
| ) from e | ||
|
|
||
| # 11. Delete original document and status. | ||
| # doc_status is deleted first so that if full_docs.delete fails, a retry | ||
| # finds no doc_status record and treats the document as already gone, | ||
| # rather than finding a doc_status that points to a missing full_docs entry. | ||
| # doc_status is deleted first so that if full_docs.delete fails, a retry | ||
| # finds no doc_status record and treats the document as already gone. | ||
| # When skip_rebuild is set the caller handles a deferred rebuild that may | ||
| # fail -- keep doc_status alive so the user can re-trigger deletion later. | ||
| try: | ||
| deletion_stage = "delete_doc_entries" | ||
| in_final_delete_stage = True | ||
| await self.doc_status.delete([doc_id]) | ||
| if not skip_rebuild: | ||
| await self.doc_status.delete([doc_id]) | ||
| await self.full_docs.delete([doc_id]) | ||
|
Comment on lines
+3971
to
3973
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| except Exception as e: | ||
| logger.error(f"Failed to delete document and status: {e}") | ||
| raise Exception(f"Failed to delete document and status: {e}") from e | ||
|
|
||
| deletion_fully_completed = True | ||
| return DeletionResult( | ||
|
|
||
| result = DeletionResult( | ||
| status="success", | ||
| doc_id=doc_id, | ||
| message=log_message, | ||
| status_code=200, | ||
| file_path=file_path, | ||
| ) | ||
| if skip_rebuild: | ||
| result.entities_to_rebuild = entities_to_rebuild | ||
| result.relationships_to_rebuild = relationships_to_rebuild | ||
| return result | ||
|
|
||
| except Exception as e: | ||
| original_exception = e | ||
|
|
||
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.
In the rebuild-failure branch, the code logs a warning but never reclassifies the previously
successful_deletionsas failed. Thefinallyblock then emits a completion summary from these counters, so users can see a fully successful batch even though doc cleanup was intentionally withheld due to rebuild failure; this makes operational status inaccurate and can hide that manual retry is still required.Useful? React with 👍 / 👎.