feat(index): optional Milvus derived-index backend behind a typed port - #431
Closed
gloryfromca wants to merge 10 commits into
Closed
feat(index): optional Milvus derived-index backend behind a typed port#431gloryfromca wants to merge 10 commits into
gloryfromca wants to merge 10 commits into
Conversation
Signed-off-by: Cheney Zhang <chen.zhang@zilliz.com>
Signed-off-by: Cheney Zhang <chen.zhang@zilliz.com>
Builds on the Milvus derived-index work in #332. That branch introduced the Predicate AST, the portable schema, and the Milvus repository, and migrated the filter DSL and the recallers off rendered LanceDB SQL. This commit restructures the layer those pieces sit in. Three gaps motivated it. The repository surface did not include the maintenance operations, so the cascade worker could not schedule optimize / prune / rebuild without knowing which backend was active. Backfill still reached LanceDB directly for its null-vector scan and its cluster-phase full-table read, so it could not run against any other backend. And there was no typed port a second adapter could be checked against. What changes: * `index/protocols.py` — IndexRepository and IndexBackend, the typed ports both adapters implement. Maintenance is deliberately part of the repository contract: an embedded engine does physical work, a service-managed engine implements a successful no-op, and the worker stays free of backend branches. * `index/router.py` — stable repository identities whose backend is resolved per call, so importers hold one object for the process lifetime while configuration still decides where reads and writes land. * `index/lancedb.py` — the LanceDB adapter behind the same port, including the predicate renderer that used to live in `lancedb/predicate.py`. * `index/schema.py` and `index/__init__.py` — reduced to the portable schema derivation and the facade the outer layers import, now that contracts and routing have their own modules. * Backfill reads through `scan()` and `repo_for_schema()`; the cascade worker looks for `index_repo`; the BM25 helpers move from `recall/base.py` into the Lance adapter that owns that syntax. Markdown remains the source of truth and SQLite remains the system-state store. This boundary owns only the rebuildable business indexes. Co-Authored-By: Cheney Zhang <chen.zhang@zilliz.com>
Follow-up to the derived-index framework review. Behaviour-preserving except where the refactor had itself changed behaviour. Restore what the refactor changed by accident: * Drop _MAX_FACT_RECALL_LIMIT. A Milvus/Zilliz topK ceiling had been hard-coded into memory/search, which also shrank LanceDB's fact expansion pool from the caller's own limit down to 1024. The ceiling belongs to whichever adapter needs it. * Restore per-column BM25 concurrency. The move into the Lance adapter turned the recallers' asyncio.gather into a sequential loop, doubling the BM25 leg on the four dual-column kinds. Close gaps the port left open: * all_of() / any_of() reject an empty result instead of silently rendering to "" — an empty filter matches every row, and for OR it also inverts the meaning. Mirrors one_of()'s existing guard. * Give the five kind-specific repository methods real Protocols (EpisodeIndexRepository / AgentSkillIndexRepository) so the router narrows _repo() by cast instead of getattr + type: ignore. * Extend the import-privacy contract to index.** and milvus.**, the only persistence subpackages it did not cover, and route the four outer-layer modules that reached inside through the facade. * search(vector=...) delegates to dense_search so the metric is cosine on both backends rather than LanceDB's default L2. * Restore the return annotations that were replaced by # type: ignore[no-untyped-def], and fix two `where: str` parameters that now receive a Predicate. Assert the filter DSL against the predicate tree it actually produces. The tests had been kept green by re-rendering the AST through the LanceDB renderer, so the compiler's own output was never asserted and the domain suite imported a physical adapter. Escaping assertions move to the adapter contract test, where quoting belongs. Drop the dead _merge_bm25_results and the compile_filters_for_backends alias. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three defects in the Milvus derived-index adapter, found reviewing the framework change. datetime round-trip below the millisecond heuristic. _to_milvus_record always writes epoch milliseconds, but _restore_row read them back through from_timestamp, which treats anything under 1e12 as *seconds*. Every instant before 2001-09-09 came back in the year 30000, or raised "ValueError: year N is out of range" on the way out. Adds from_timestamp_ms as the exact inverse of to_timestamp_ms and reads the physical column through it. Lost update in update(). Milvus has no partial-column update, so the row is read back and re-upserted whole — but only the upsert held the write lock. Concurrent writers to one row (backfill setting `vector`, reflection setting `deprecated_by`) each overwrote the other's column with the value they had read before it landed. Both halves now hold the lock. This also removes the 10,000-row fetch ceiling that used to truncate silently and still report success; the read goes through the iterator instead. list_by_owner_after_ts overran the query window. It asked client.query for up to 20,000 rows, which cannot cross Milvus' 16,384-row result window — the same limit this package documents in _scan_raw and works around there. It now applies its ceiling through the iterator. Also stops _reset_collection_cache from clearing _collection_locks while other tasks may hold them, and drops the module-level verify_business_schemas, which had no caller. Tests: a unit round-trip over four instants spanning the 1e12 threshold (runs without a server), and two remote cases — update on a row that carries vectors, which only user_profile covered before, and a pre-2001 timestamp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
verify_collection() compared field *names* only, so a collection whose columns happened to share our names but carried the wrong datatype, primary key, nullability or vector dimension passed startup and then failed opaquely on the first write or search. That is the shape a stale model or a reused collection_prefix produces. Creation and verification now share one _PhysicalField descriptor, so a reported mismatch always means the server disagrees with us — never that the builder and the checker have drifted apart. The created schema is byte-identical to before for all seven tables. What is strict and what is not. describe_collection omits `nullable` and `is_primary` when false and omits `element_type` for non-array columns, so absence is unambiguous and those are compared exactly, along with datatype and vector dimension — the drift that fails silently and late. max_length and max_capacity are advisory: the server may normalize them, and a wrong length already surfaces as a loud write rejection, so blocking startup on them would trade a small gap for an outage risk. They are logged, never swallowed. Index and metric verification is deliberately not included. It needs a second descriptor shape (describe_index) that cannot be confirmed without a live server, and shipping an unverifiable check here would risk exactly the false positive this change is careful to avoid. Tests. The offline tests build their fake describe_collection reply from the same descriptor the verifier reads, so they can only prove creation and verification agree with each other; a hand-written snapshot of episode's physical layout is the independent statement of intent, and foresight.evidence_tokens — the only column that is both nullable and a BM25 input — pins the interaction neither episode nor user_profile can reach. Whether the descriptor matches what Milvus actually reports is asserted in the remote suite, which re-verifies every collection after clearing the process readiness cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gloryfromca
force-pushed
the
feat/derived-index-backends
branch
from
September 3, 2026 06:21
b837536 to
f99f4cc
Compare
The end-to-end suites drove LanceDB only, so nothing had ever exercised Milvus through the request path -- /add, /flush, cascade, the search methods, /get. The repository contract tests cover the port; they say nothing about whether the application works on top of it. Three of the tier modules carried the same helper reading episode rows straight off a Lance table. It moves into the conftest as episode_rows() and reads through the index facade instead, which is all the coupling that stood in the way -- the remaining direct LanceDB use in test_cascade_cli_integration inspects an Arrow column type and is genuinely engine-specific, so it stays. An index_backend fixture parametrizes the tier runtimes. LanceDB always runs; Milvus joins only when EVEROS_TEST_MILVUS_URI names a real server, because it is a remote service with no embedded fallback to degrade to -- a run without one proves nothing about Milvus and should not collect cases that suggest otherwise. Each Milvus case gets its own collection prefix and drops it afterwards, since collections outlive the process. Verified against Milvus 2.6.22: all 27 parametrized cases pass, though not in a single process on a 5.8 GiB Docker allocation -- 23 pass and the server is killed under the collection churn, and the remaining 4 pass on a restart. The CI job has more headroom; the new step there is what proves it holds in one run. Co-Authored-By: Cheney Zhang <chen.zhang@zilliz.com>
Collaborator
Author
|
Superseded by #332 — the same nine commits now live on that branch, pushed as a fast-forward so @zc277584121's five commits keep their SHAs and authorship. Consolidating there rather than here because collaboration needs push access: on this PR that would have meant repo-wide write for an external contributor, whereas on their own fork they already have it. #332 stays their PR and we can both push to it. |
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.
Summary
Adds an optional Milvus backend for the rebuildable derived index, behind a
typed port that LanceDB also implements.
This continues @zc277584121's work in #332 rather than replacing it. That
branch is included here commit-for-commit — it introduced the Predicate AST,
the portable schema derivation, the Milvus repository, and migrated the filter
DSL and the recallers off rendered LanceDB SQL. The four commits on top
restructure the layer those pieces sit in and fix defects found reviewing it.
What the follow-up commits change
refactor(index): split the derived-index port into contracts, routing, and a Lance adapter— three gaps motivated it. The repository surface did notinclude maintenance operations, so the cascade worker could not schedule
optimize / prune / rebuild without knowing the active backend. Backfill still
reached LanceDB directly for its null-vector scan and its cluster-phase
full-table read. And there was no typed port a second adapter could be checked
against. Adds
index/protocols.py(the ports),index/router.py(stablerepository identities, backend resolved per call), and
index/lancedb.py(theLanceDB adapter, including the predicate renderer).
refactor(index): stop the port leaking backend limits and syntax— aMilvus/Zilliz topK ceiling had been hard-coded into
memory/search, which alsoshrank LanceDB's fact-expansion pool to 1024; per-column BM25 lost its
asyncio.gather. Both restored. Emptyall_of()/any_of()now raiseinstead of rendering to
""(an empty filter matches every row, and for OR itinverts the meaning). The import-privacy contract is extended to
index.**and
milvus.**, and the filter-DSL tests now assert against the predicate treeinstead of re-rendering it through the LanceDB adapter.
fix(milvus): correct the datetime round-trip, update race, and scan window—to_timestamp_mswrites milliseconds unconditionally, but the read path usedfrom_timestamp, which treats anything under 1e12 as seconds: every instantbefore 2001-09-09 came back in the year 30000 or raised.
update()didread-modify-write with only the upsert under the write lock, so concurrent
writers to one row erased each other's columns.
list_by_owner_after_tsaskedclient.queryfor 20,000 rows, which cannot cross Milvus' 16,384-row window.fix(milvus): verify the physical collection schema, not just field names—verify_collection()compared field names only, so a collection whosecolumns shared our names but carried the wrong datatype, primary key,
nullability or vector dimension passed startup and failed opaquely on the first
write. Creation and verification now share one descriptor; datatype, primary,
nullable, dimension and element type are strict, while VARCHAR/array limits are
advisory (the server may normalize them, and a wrong length already fails
loudly on write).
Area
Verification
Checklist
main..envfiles, dependency folders, or generated output.Notes for Reviewers
Please review #332's commits and the follow-ups separately. The physical
Milvus adapter (
infra/persistence/milvus/**) is @zc277584121's work and isbest reviewed by someone who knows Milvus; the port, routing and Lance adapter
are the EverOS side.
Four remote Milvus tests have never run. The
integration-milvusjob isthe first real execution of
test_milvus_remote.pyfor this branch, and threeof its cases are new here:
update()on a row that carries vectors (previouscoverage used
user_profile, which has no vector column), a pre-2001 timestampround-trip, and re-verifying every collection after clearing the process
readiness cache. That last one is the only check that can confirm the physical
schema descriptor matches what Milvus actually reports — every offline test
builds its fake reply from the same descriptor the verifier reads, so those can
only prove creation and verification agree with each other.
Deliberately not included:
COSINE, BM25 output fields). It needsa second descriptor shape (
describe_index) that could not be confirmedwithout a live server, and an unverifiable check risks exactly the false
positive the rest of this change is careful to avoid.
infra/persistence/lancedb. The Milvusadapter still imports them from there, so the schema is not yet
backend-neutral in the strict sense. Doing it here would change
to_arrow_schema()output and force a rebuild for existing deployments.only; it is not a Milvus long-run proof.
Known limits carried forward:
update()on Milvus now holds the write lockacross its read, which serializes updates against other writes on that
collection;
scan()returns a list, so a very large backfill materializes inmemory; the pagination contract still tops out at a 20,000-candidate window.