Repository navigation
Collect the same fuzzy-index terms on both ingestion paths (#346) - #360
Open
Bernhard Merkle (bmerkle) wants to merge 1 commit into
Open
Bernhard Merkle (bmerkle) wants to merge 1 commit into
Bernhard Merkle (bmerkle) wants to merge 1 commit into
Conversation
…#346) add_messages_with_indexing (ConversationBase._update_related_terms_incremental) only embedded entity names, topic text and individual verbs, while add_messages_streaming used collect_entity_terms / collect_action_terms and also embedded types, facets, subjects, objects and params. So related-term expansion depended on the ingestion path. The incremental path also embedded each verb on its own, while the term index key is the joined verbs, and the streaming path embedded types and facets of nameless entities, which the term index skips. The streaming path also never embedded terms from message metadata (e.g. speakers and recipients), which are indexed at commit time. Add semantic_ref_index.collect_knowledge_terms and collect_related_terms_for_fuzzy_index, which return exactly the normalized, deduplicated terms a knowledge item adds to the semantic ref index, and use them on both paths. The streaming commit now also embeds the terms of the new semantic refs that have no precomputed embedding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Kevin Turcios (@KRRT7) could you take a look? This is the other half of #346: both ingestion paths now add the same terms to the fuzzy related-terms index, including metadata terms on the streaming path. Independent of #358. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Commit-time embedding failures can leave retries with missing persisted terms, and nameless metadata entities still produce inconsistent index contents.
2 open issues
What changed in this PR
Addresses #346’s ingestion-path inconsistency by sharing fuzzy-index term collection between incremental and streaming ingestion.
Changes:
- Centralizes normalized, deduplicated knowledge-term collection.
- Adds streaming metadata terms at commit time.
- Adds term-parity regression tests and documents the behavior change.
| File | Description |
|---|---|
| tests/test_add_messages_with_indexing.py | Tests incremental ingestion term parity. |
| tests/test_add_messages_streaming.py | Tests streaming term parity, including metadata. |
| tests/test_add_messages_pipeline.py | Tests shared collectors against semantic-index terms. |
| src/typeagent/storage/memory/semantic_ref_index.py | Adds shared knowledge-term collectors. |
| src/typeagent/knowpro/conversation_base.py | Uses shared collection and embeds remaining metadata terms. |
| src/typeagent/knowpro/add_messages.py | Delegates pipeline collection to the shared helper. |
| CHANGELOG.md | Documents ingestion behavior and embedding changes. |
Review effort: Balanced
Comment on lines
+442
to
+444
| await self._update_related_terms_incremental( | ||
| start_points.semref_count, exclude=set(related_terms) | ||
| ) |
Comment on lines
+590
to
+591
| if isinstance(knowledge, ConcreteEntity): | ||
| return collect_entity_terms(knowledge) if validate_entity(knowledge) else [] |
This branch has not been deployed
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.
Part of #346 (item 3). Items 1 and 2 are in #358; the two PRs are independent and can merge in either order. Together they fix #346.
The fuzzy related-terms index got different terms depending on how messages were ingested, so related-term expansion depended on the ingestion path:
add_messages_with_indexing(before)add_messages_streaming(before)"looks","up")"looks up"), like the term indexChange
semantic_ref_index.collect_knowledge_termsandcollect_related_terms_for_fuzzy_indexreturn exactly the normalized, deduplicated terms a knowledge item adds to the semantic ref index.ConversationBase._update_related_terms_incrementalandadd_messages._collect_related_terms_for_fuzzy_indexboth use them._update_related_terms_incremental(..., exclude=<precomputed terms>)after adding the precomputed ones, which embeds only the remaining (metadata) terms.After this, on both paths the fuzzy index holds the same terms as the semantic ref index, which is also what
build_related_terms_indexproduces when rebuilding from scratch.Cost:
add_messages_with_indexingembeds more terms per message, andadd_messages_streamingembeds metadata terms at commit time. Repeated terms are served from the embedding cache (memory) or skipped as already stored (SQLite).Tests (each fails on
main):test_fuzzy_terms_match_semantic_ref_index_terms: both collectors return exactly the term-index terms for knowledge with multi-word verbs, a nameless entity, an inverse action, params and facets.test_add_messages_with_indexing_fuzzy_terms_match_term_index(with and without knowledge extraction) andtest_streaming_fuzzy_terms_match_term_index: after ingesting on SQLite, fuzzy-index terms equal semantic-ref-index terms.Not addressed (pre-existing): the in-memory
TermEmbeddingIndex.add_terms_with_embeddingsdoesn't skip terms it already holds, so repeated batches can store duplicate vectors; this change makes that more frequent for common terms such asperson.Testing:
makelocally: format, ruff and pyright (3.12 and 3.15) clean, build OK; 912 passed. 7 tests fail locally because of a brokenjiternative module on my machine (plus an MCP sampling test that depends on it); they fail identically onmain, so CI is the reference for those.🤖 Generated with Claude Code