perf: enrich knowledge base embeddings with document context - #9457
Open
lxfight wants to merge 3 commits into
Open
perf: enrich knowledge base embeddings with document context#9457lxfight wants to merge 3 commits into
lxfight wants to merge 3 commits into
Conversation
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
FaissVecDB.insert_batch, the debug log still reportslen(contents)while embeddings are now generated fromembedding_contents; consider logging the actual embedding input count to avoid confusion when these lists differ. - The new
embedding_contentsparameter was added to theVecDBbase interface; double-check that all concreteVecDBimplementations accept and correctly handle this argument to prevent runtime errors or silently ignored enrichment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `FaissVecDB.insert_batch`, the debug log still reports `len(contents)` while embeddings are now generated from `embedding_contents`; consider logging the actual embedding input count to avoid confusion when these lists differ.
- The new `embedding_contents` parameter was added to the `VecDB` base interface; double-check that all concrete `VecDB` implementations accept and correctly handle this argument to prevent runtime errors or silently ignored enrichment.
## Individual Comments
### Comment 1
<location path="astrbot/core/knowledge_base/kb_helper.py" line_range="395" />
<code_context>
"chunk_index": idx,
},
)
+ document_title = Path(file_name).stem.strip()
+ embedding_contents = (
+ [f"{document_title}\n\n{chunk_text}" for chunk_text in chunks_text]
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `file_name` being `None` before constructing a `Path`.
Some call paths may pass `file_name=None` or omit it, and `Path(None)` will raise a `TypeError`. Please add a guard (e.g. `document_title = Path(file_name).stem.strip() if file_name else ""`) or ensure the title is derived earlier when `file_name` is guaranteed to be set.
</issue_to_address>
### Comment 2
<location path="tests/unit/test_faiss_vec_db.py" line_range="68-77" />
<code_context>
vec_db.embedding_storage.insert_batch.assert_not_awaited()
+@pytest.mark.asyncio
+async def test_insert_batch_embeds_context_but_stores_original_contents() -> None:
+ vec_db = FaissVecDB.__new__(FaissVecDB)
+ vec_db.embedding_provider = AsyncMock()
+ vec_db.embedding_provider.get_embeddings_batch.return_value = [
+ [0.1, 0.2],
+ [0.3, 0.4],
+ ]
+ vec_db.document_storage = AsyncMock()
+ vec_db.document_storage.insert_documents_batch.return_value = [11, 12]
+ vec_db.embedding_storage = AsyncMock()
+ vec_db.embedding_storage.dimension = 2
+
+ await FaissVecDB.insert_batch(
+ vec_db,
+ contents=["chunk one", "chunk two"],
+ metadatas=[{}, {}],
+ ids=["doc-1", "doc-2"],
+ embedding_contents=["guide\n\nchunk one", "guide\n\nchunk two"],
+ )
+
+ vec_db.embedding_provider.get_embeddings_batch.assert_awaited_once_with(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for the default `embedding_contents=None` path and the new mismatch error condition.
To fully cover the new `insert_batch` behavior, please add:
1. A test where `embedding_contents` is omitted or `None`, verifying that `get_embeddings_batch` is called with `contents` and `insert_documents_batch` receives the original `contents`.
2. A test where `embedding_contents` and `contents` differ in length, verifying that a `KnowledgeBaseUploadError` is raised with the expected `stage` and `details`.
These will explicitly exercise both the default path and the new safety check and help prevent subtle caller bugs.
Suggested implementation:
```python
["doc-1", "doc-2"],
["chunk one", "chunk two"],
[{}, {}],
)
@pytest.mark.asyncio
async def test_insert_batch_uses_contents_when_embedding_contents_not_provided() -> None:
vec_db = FaissVecDB.__new__(FaissVecDB)
vec_db.embedding_provider = AsyncMock()
vec_db.embedding_provider.get_embeddings_batch.return_value = [
[0.5, 0.6],
[0.7, 0.8],
]
vec_db.document_storage = AsyncMock()
vec_db.document_storage.insert_documents_batch.return_value = [21, 22]
vec_db.embedding_storage = AsyncMock()
vec_db.embedding_storage.dimension = 2
await FaissVecDB.insert_batch(
vec_db,
contents=["default one", "default two"],
metadatas=[{"m": 1}, {"m": 2}],
ids=["doc-3", "doc-4"],
# omit embedding_contents to take the default path
)
vec_db.embedding_provider.get_embeddings_batch.assert_awaited_once_with(
["default one", "default two"],
batch_size=32,
tasks_limit=3,
max_retries=3,
progress_callback=None,
)
vec_db.document_storage.insert_documents_batch.assert_awaited_once_with(
["doc-3", "doc-4"],
["default one", "default two"],
[{"m": 1}, {"m": 2}],
)
@pytest.mark.asyncio
async def test_insert_batch_raises_on_embedding_contents_length_mismatch() -> None:
vec_db = FaissVecDB.__new__(FaissVecDB)
vec_db.embedding_provider = AsyncMock()
vec_db.document_storage = AsyncMock()
vec_db.embedding_storage = AsyncMock()
vec_db.embedding_storage.dimension = 2
with pytest.raises(KnowledgeBaseUploadError) as excinfo:
await FaissVecDB.insert_batch(
vec_db,
contents=["chunk one", "chunk two"],
metadatas=[{}, {}],
ids=["doc-1", "doc-2"],
embedding_contents=["guide\n\nchunk one"],
)
# Ensure we fail fast with a clear stage and details
assert excinfo.value.stage == "embedding"
assert "length mismatch" in excinfo.value.details
```
1. Ensure `KnowledgeBaseUploadError` is imported at the top of `tests/unit/test_faiss_vec_db.py` (e.g., `from <module> import KnowledgeBaseUploadError`) consistent with the rest of the codebase.
2. Confirm that `FaissVecDB.insert_batch`:
- Defaults `embedding_contents` to `contents` when `embedding_contents` is `None` or omitted.
- Raises `KnowledgeBaseUploadError` on length mismatch with `stage="embedding"` and a `details` string that includes "length mismatch" (or adjust the assertion to match the actual message).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
Reviewed the remaining high-level suggestions:
The actionable test-coverage suggestion is addressed in 97b026b. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short knowledge base chunks often lose the document and section context needed to match user queries. This PR enriches embedding inputs with the document title and preserves heading paths for formats that are parsed into Markdown, while keeping vector storage and returned chunk behavior compatible.
Modifications / 改动点
Handbook > Installationfor nested sections.Screenshots or Test Results / 运行截图或测试结果
Document title experiment
d921ec7e349ce0d28daf30b2da9da5ee698bef0dastrbot-kb-context-v1baai/bge-m3Held-out fusion metrics:
Heading path and adjacent context experiment
To recover real heading paths, MIRACL passages were joined with public Chinese Wikipedia revisions. The candidate set contained all 22,230 passages from the 783 relevant articles, with 393 queries and 994 positive labels. This experiment is a separate corpus and its absolute scores are not directly comparable to the title experiment.
Held-out fusion metrics:
The heading-only variant was selected on the development split and improved every held-out top-five metric. Adjacent context diluted the target passage and was intentionally excluded. Benchmark datasets and generated results remain local and are not included in this PR.
Verification steps:
Results:
Checklist / 检查清单
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。/ 我的更改没有引入恶意代码。
Summary by Sourcery
Enrich knowledge base embeddings with document and section context while keeping stored chunks unchanged and maintaining upload atomicity guarantees.
New Features:
Bug Fixes:
Tests: