Repository navigation
Python: add Milvus vector store (agent-framework-milvus) - #9346
Nantha kumar (Nanduu24) wants to merge 12 commits into
Conversation
Adds a new agent-framework-milvus package providing MilvusStore, MilvusCollection and MilvusSettings on the BaseVectorStore / BaseVectorCollection interfaces, so Milvus is available as a vector store alongside the Postgres, Qdrant, Redis, MongoDB and other connectors. - The data model definition drives the collection schema: the key field becomes the primary key (INT64/VARCHAR), data fields become typed scalar fields, and vector fields become FLOAT_VECTOR fields, each with an index. - Distance functions map to Milvus metrics (COSINE/IP/L2) and index kinds to Milvus index types (HNSW/FLAT/IVF_FLAT/DISKANN/AUTOINDEX). - Portable filters (eq, ne, gt, gte, lt, lte, between, in, not_in and and/or/not groups) are translated to Milvus boolean expressions; connector-side score thresholds respect each metric's comparison direction. - Upsert, get (by key or filter), delete, vector search and collection lifecycle, with the sync MilvusClient wrapped in asyncio.to_thread. - README, a runnable sample, and unit tests (filters, settings, collection) plus integration tests gated behind MILVUS_TEST_URI. Registered in the workspace.
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
documentation
There was a problem hiding this comment.
🟡 Changes recommended
Connector correctness, package registration, CI coverage, integration testing, and documentation issues remain unresolved.
9 open findings
What changed in this PR
Adds a first-party Milvus vector-store connector implementing Agent Framework’s collection, store, filtering, lifecycle, and vector-search abstractions.
Changes:
- Adds Milvus schema, CRUD, filtering, search, settings, and client ownership support.
- Adds package documentation, samples, and unit-test scaffolding.
- Registers and locks the new workspace package and dependencies.
| File | Description |
|---|---|
python/uv.lock |
Locks Milvus and transitive dependencies. |
python/pyproject.toml |
Registers the workspace source. |
python/packages/milvus/tests/milvus/test_settings.py |
Tests settings resolution. |
python/packages/milvus/tests/milvus/test_filters.py |
Tests filter translation. |
python/packages/milvus/tests/milvus/test_collection.py |
Tests collection behavior. |
python/packages/milvus/tests/milvus/conftest.py |
Defines test fixtures. |
python/packages/milvus/samples/README.md |
Documents the sample. |
python/packages/milvus/samples/milvus_vectors.py |
Demonstrates Milvus usage. |
python/packages/milvus/README.md |
Documents installation and APIs. |
python/packages/milvus/pyproject.toml |
Defines package metadata and tasks. |
python/packages/milvus/LICENSE |
Adds the MIT license. |
python/packages/milvus/agent_framework_milvus/py.typed |
Marks typed-package support. |
python/packages/milvus/agent_framework_milvus/_vector_store.py |
Implements the connector. |
python/packages/milvus/agent_framework_milvus/__init__.py |
Exports public APIs. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- ensure_collection_exists: handle concurrent creation (treat an already-created collection as success instead of failing the ensure operation). - Filter string literals are serialized with json.dumps so quotes, backslashes and control characters cannot break out of the Milvus expression. - Reject is_indexed / is_full_text_indexed on data fields until scalar/full-text index creation is implemented, rather than silently ignoring them. - Validate score_threshold as a finite, non-boolean number before searching. - Qualify the Milvus Lite (local .db) docs: it requires the pymilvus[milvus_lite] extra; fix the README usage snippet (sequence upsert, generate_vectors, subscript SearchResponse fields). - Add live integration tests exercising server_collection (CRUD, search, filters), selected by -m integration. - Add a Milvus PR unit-test job and path filter so regressions are caught on PRs. - Register the package in PACKAGE_STATUS.md (alpha / no).
GitHub Actions
Active
|
Great! Thx |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (2 commit(s)): d121cfe0e10a, 73796abec852
Model: gpt-5.6-sol
Overview
The review found 2 verified inline finding(s).
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/milvus/agent_framework_milvus/_vector_store.py
Addresses review feedback on microsoft#9346: - ensure_collection_exists now validates an existing collection's fields, vector dimensions and metric types against the data model definition (and does the same after an already-exists race) instead of accepting a stale or unrelated schema. - Portable filter operands are validated against the declared field type: a boolean operand on a numeric field (or any type mismatch) is rejected with a clear error rather than emitting an invalid comparison such as 'price == true'; ordered comparisons require a numeric field. - Added unit tests for schema mismatch (dimension, metric) and operand-type rejection.
GitHub Actions
Active
…on_exists Per review on microsoft#9346: create_collection builds the collection and then separately creates indexes and loads it. Treating any exception as success when the collection exists afterwards could mask an index or load failure, leaving an unindexed/unloaded collection. Now only a genuine already-exists/concurrent-create MilvusException is swallowed (then the existing collection is validated); all other create/index/load failures are re-raised. Added tests for the index/load-failure and non-Milvus cases.
GitHub Actions
Active
GitHub Actions
Active
| ) | ||
| resolved_token = settings.get("token") | ||
| resolved_password = settings.get("password") | ||
| client_kwargs: dict[str, Any] = {"uri": settings.get("uri") or DEFAULT_URI} |
There was a problem hiding this comment.
| client_kwargs: dict[str, Any] = {"uri": settings.get("uri") or DEFAULT_URI} | |
| client_kwargs: dict[str, Any] = {"uri": settings.get("uri", DEFAULT_URI)} |
There was a problem hiding this comment.
Kept settings.get("uri") or DEFAULT_URI: load_settings includes optional keys as None when unset, so settings.get("uri", DEFAULT_URI) would return None rather than the default (verified). Happy to change if you'd prefer a different shape.
| if client is not None and any( | ||
| value is not None for value in (uri, token, user, password, db_name, env_file_path, env_file_encoding) | ||
| ): | ||
| raise ValueError("Pass client or connection settings, not both.") |
There was a problem hiding this comment.
this should be handled after parsing the settings, some or all of this might come from the environment
There was a problem hiding this comment.
This mirrors the Qdrant connector's guard and intentionally inspects only the explicit constructor args — env-sourced settings are ambient and aren't treated as 'passed'. Happy to drop the guard (and let an explicit client win) if you'd prefer; let me know.
Addresses review feedback on microsoft#9346: - Switch from the sync MilvusClient wrapped in asyncio.to_thread to the native AsyncMilvusClient, awaiting operations directly. - Let the native Milvus client-creation error propagate instead of wrapping it. - Replace the brittle already-exists message match: on a create failure, validate the existing collection and re-raise the original error if it does not match, so only a genuine concurrent-create with a complete, matching collection is accepted. - Tests use MILVUS_URI (not a separate MILVUS_TEST_URI) to gate integration tests; the env-clearing fixture is skipped for integration tests. - Require pymilvus>=2.5.3 (AsyncMilvusClient).
GitHub Actions
Active
Verified pymilvus 2.5.3+ and its dependencies (grpcio, numpy, …) resolve, install and import on Python 3.15, matching the framework's new 3.15 support.
GitHub Actions
Active
GitHub Actions
Active
|
Nantha kumar (@Nanduu24) Thanks for the contribution. The package direction is already agreed, but the PR body does not link the agreed tracking issue. Please add that issue to the PR's Related Issue information so the recorded agreement and lifecycle decision are directly connected to this package addition. |
|
Thanks Eduard van Valkenburg (@eavanvalkenburg)! Linked the tracking issue — added |
GitHub Actions
Active
GitHub Actions
Active
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
This is a substantial and promising connector, but schema safety, vector validation, live integration coverage, and the PR-caused workspace CI failure need to be resolved before it is merge-ready.
| python-version: ${{ env.UV_PYTHON }} | ||
| os: ${{ runner.os }} | ||
| - name: Test Milvus package | ||
| run: uv run poe test -P milvus |
There was a problem hiding this comment.
This job runs only poe test, which excludes the newly added live integration tests. The workflow header requires parallel jobs to stay in sync with python-integration-tests.yml, but that workflow has no Milvus service/Lite job and Milvus is absent from its report/gate dependencies. Please schedule poe test-integration against a disposable Milvus server or Milvus Lite in the integration workflow and wire it into the aggregate report/check.
There was a problem hiding this comment.
Looking into this, I hit two concrete constraints and want your steer before wiring it:
- Milvus Lite can't be driven via
MILVUS_URI. pymilvus reads a globalMILVUS_URIat import and validates it ashttp(s)://..., so pointing it at a local.dbcrashesimport pymilvus. (I verifiedAsyncMilvusClientitself works fine against Lite when the path is passed as theuri=argument, not the env var — I've fixed the docs accordingly.) - No single-container Milvus service. Milvus standalone needs etcd+MinIO (or embedded etcd via
milvus run standalone), and GitHubservices:can't override the image command, so it can't be a service container like qdrant/redis/postgres — it needs adocker runstep.
Proposed: add python-tests-milvus to python-integration-tests.yml that docker runs a disposable Milvus standalone (embedded etcd) with MILVUS_URI=http://localhost:19530, waits on /healthz, runs poe test-integration, and wire it into the report + python-integration-tests-check gate. Happy to push that — just confirming you'd prefer the standalone-server route over Milvus Lite (which the reserved env var rules out for the MILVUS_URI-gated tests).
…r-store # Conflicts: # python/uv.lock
GitHub Actions
Active
Addresses review feedback on microsoft#9346: - Existing-collection validation now also checks the primary-key type, every data/vector field DataType, and the (non-AUTOINDEX) vector index kind, in addition to dimensions and metric, so a same-named but incompatible collection is rejected. Added key/data/vector type and index-kind mismatch tests. - Added a shared dense-vector validator used for both stored and query vectors: rejects non-numeric sequences (incl. strings like "123"), booleans, and NaN/infinity or out-of-float32-range values, instead of coercing with float(). Added regression tests. - Removed the now-unused ty: ignore[unresolved-import] on the unrelated optimized_data_formats sample (pymilvus pulls pandas into the workspace, so ty resolves the import), fixing the PR-caused Samples/Typing CI failures. - Clarified docs: Milvus Lite is selected via the uri= argument with a local .db path; the MILVUS_URI env var must be a server address (pymilvus reserves it).
GitHub Actions
Active
GitHub Actions
Active
|
Heads-up on the red |
GitHub Actions
Active
|
Hi Eduard van Valkenburg (@eavanvalkenburg) — thanks for merging #9614. With that fix on The code-review items from your last pass are all addressed and pushed — schema safety (strict validation of an existing collection's key/primary/data/vector field types, dims, and metric), dense-vector validation (rejecting NaN/inf/overflow/non-numeric), and the concurrent-create handling. The one item still awaiting your steer is live integration coverage. Milvus Lite can't be driven through |
Summary
Adds a new
agent-framework-milvuspackage providing a Milvus vector store for Agent Framework, implementing theBaseVectorStore/BaseVectorCollection/BaseVectorSearchinterfaces. This makes Milvus — one of the most widely used open-source vector databases — available out of the box alongside the existing Postgres, Qdrant, Redis, MongoDB, SQL Server and other vector-store packages.It follows the structure of the existing
agent-framework-qdrantpackage (settings viaload_settings, a store that mints collection clients, borrowed-vs-managed client ownership,operation_optionsvalidation, native filtering).What's included
MilvusStore/MilvusCollection/MilvusSettings(+py.typed).INT64forintkeys,VARCHARotherwise), data fields become typed scalar fields, and vector fields becomeFLOAT_VECTORfields, each with its own index.DistanceFunction→ Milvus metric types (cosine_similarity/cosine_distance→COSINE,dot_prod→IP,euclidean_distance/euclidean_squared_distance→L2);IndexKind→ Milvus index types (hnsw,flat,ivf_flat,disk_ann,default→AUTOINDEX).ensure_collection_exists/collection_exists/ensure_collection_deleted,upsert,get(by key or portable filter),delete, and vectorsearch.eq,ne,gt,gte,lt,lte,between,in,not_in, andand/or/notgroups. Operators that Milvus can't implement with correct null/collection semantics raiseNotImplementedErrorrather than silently returning wrong results.score_thresholdthat respects each metric's comparison direction (larger-is-closer forCOSINE/IP, smaller-is-closer forL2), since core does not post-filter.MilvusClientis wrapped inasyncio.to_thread, which keeps the connector compatible across the supportedpymilvusrange while presenting the framework's async interface.samples/milvus_vectors.py), and the package registered in the workspace[tool.uv.sources].Testing
tests/milvus/): filter translation, settings resolution/precedence, and collection behavior (upsert/get/delete/search, schema building, serialization, lifecycle, client ownership) — hermetic, no live service.MILVUS_TEST_URIfor a real server.ruff check,ruff format --check, and Pyright (strict) are clean on the package;uv lock --lockedsucceeds.Motivation
Milvus is a top-tier open-source vector database with no first-party Agent Framework integration. This PR implements the accepted feature request in #3488, with a connector that matches the conventions of the other vector-store packages.
Related issue
Closes #3488 — the agreed Milvus VectorStore connector feature request.