Skip to content

feat(index): optional Milvus derived-index backend behind a typed port - #431

Closed
gloryfromca wants to merge 10 commits into
mainfrom
feat/derived-index-backends
Closed

feat(index): optional Milvus derived-index backend behind a typed port#431
gloryfromca wants to merge 10 commits into
mainfrom
feat/derived-index-backends

Conversation

@gloryfromca

Copy link
Copy Markdown
Collaborator

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 not
include 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 (stable
repository identities, backend resolved per call), and index/lancedb.py (the
LanceDB adapter, including the predicate renderer).

refactor(index): stop the port leaking backend limits and syntax — a
Milvus/Zilliz topK ceiling had been hard-coded into memory/search, which also
shrank LanceDB's fact-expansion pool to 1024; per-column BM25 lost its
asyncio.gather. Both restored. Empty all_of() / any_of() now raise
instead of rendering to "" (an empty filter matches every row, and for OR it
inverts the meaning). The import-privacy contract is extended to index.**
and milvus.**, and the filter-DSL tests now assert against the predicate tree
instead of re-rendering it through the LanceDB adapter.

fix(milvus): correct the datetime round-trip, update race, and scan window
to_timestamp_ms writes milliseconds unconditionally, but the read path used
from_timestamp, which treats anything under 1e12 as seconds: every instant
before 2001-09-09 came back in the year 30000 or raised. update() did
read-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_ts asked
client.query for 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 whose
columns 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

  • Architecture method
  • Benchmark
  • Use case
  • Documentation
  • Developer experience
  • CI, build, or release

Verification

make lint          all checks passed; 3 import contracts kept
make test          2118 passed
make integration   183 passed, 4 skipped, 7 deselected

The 4 skips are tests/integration/test_milvus_remote.py, which needs
EVEROS_TEST_MILVUS_URI. They have NOT been run locally — no Milvus was
available. The integration-milvus job in this PR is their first execution.

Checklist

  • I kept the change scoped to the relevant area.
  • I am opening this from a separate branch, not pushing directly to main.
  • I updated docs, examples, or setup notes when behavior changed.
  • I added or updated tests when the change affects behavior.
  • I did not commit secrets, .env files, dependency folders, or generated output.
  • Active relative links in Markdown files resolve.

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 is
best 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-milvus job is
the first real execution of test_milvus_remote.py for this branch, and three
of its cases are new here: update() on a row that carries vectors (previous
coverage used user_profile, which has no vector column), a pre-2001 timestamp
round-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:

  • Index and metric verification (dense COSINE, BM25 output fields). It needs
    a second descriptor shape (describe_index) that could not be confirmed
    without a live server, and an unverifiable check risks exactly the false
    positive the rest of this change is careful to avoid.
  • Moving the record models out of infra/persistence/lancedb. The Milvus
    adapter 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.
  • Milvus soak testing. The 18,000-second soak in this work covered LanceDB
    only; it is not a Milvus long-run proof.

Known limits carried forward: update() on Milvus now holds the write lock
across its read, which serializes updates against other writes on that
collection; scan() returns a list, so a very large backfill materializes in
memory; the pagination contract still tops out at a 20,000-candidate window.

zhanghui and others added 4 commits September 3, 2026 14:21
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
gloryfromca force-pushed the feat/derived-index-backends branch from b837536 to f99f4cc Compare September 3, 2026 06:21
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>
@gloryfromca

Copy link
Copy Markdown
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.

@gloryfromca gloryfromca closed this Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants