{{ message }}
fix: Fix five bugs in milvus online store#6275
Merged
franciscojavierarceo merged 7 commits intoApr 15, 2026
Merged
Conversation
Contributor
Sorry, something went wrong.
Add missing RST files for feast.aggregation (including tiling sub-package), feast.dbt, and feast.openlineage. Also add feast.diff.apply_progress which was missing from feast.diff.rst. Update feast.rst toctree to include all three new top-level packages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>
No new warning categories are introduced by adding feast.aggregation, feast.dbt, and feast.openlineage to the Sphinx docs. The 10 new "more than one target found" warnings follow the same pre-existing pattern (8 already present) caused by feast.__init__ re-exporting classes under multiple paths. Fixes applied: - Remove :undoc-members: from dataclass automodules (Aggregation, IRMetadata, OpenLineageConfig) to prevent duplicate attribute docs - Document re-exporting __init__ packages at package level only to avoid duplicate descriptions from submodule entries - Fix markdown-style code fences in openlineage/__init__.py docstring - Fix RST formatting in client.py Example section (Example::) and emitter.py ASCII art diagram (literal block) - Fix repo_config.py field docstring that triggered an ambiguous OpenLineageConfig cross-reference Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>
1. update() overwrote self._collections with the raw describe_collection() dict from the last table instead of updating the keyed cache entry. _get_or_create_collection() already updates self._collections as a side effect, so the assignment is simply dropped. 2. plan() raised NotImplementedError instead of returning [] like the base class default. This caused `feast plan` to crash for Milvus stores. 3. online_read() carried an extra full_feature_names parameter not present in the OnlineStore base class, violating the interface contract. The parameter was unused and is removed. 4. retrieve_online_documents_v2() passed the raw hit.get() result (which can be None when the composite key is absent) directly to bytes.fromhex(), raising TypeError. Guard added: only call bytes.fromhex() when the value is non-None. 5. Replace print() calls in _connect() with logger.info() so connection messages respect standard logging configuration instead of always writing to stdout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1. test_milvus_update_preserves_collection_cache — verifies that applying two FeatureViews leaves self._collections as a proper dict keyed by collection name, with a "fields" entry for each view. Previously, update() overwrote _collections with the raw describe_collection() dict from the last table. 2. test_milvus_plan_returns_empty_list — verifies that plan() returns [] instead of raising NotImplementedError, matching the OnlineStore base class default and allowing `feast plan` to work. 3. test_milvus_retrieve_online_documents_v2_missing_entity_key — verifies that a search hit missing the composite primary key produces a None entity_key_proto instead of a TypeError from bytes.fromhex(None). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>
franciscojavierarceo
approved these changes
Apr 15, 2026
Contributor
Author
|
Hmm, looks like the unit test error for python 3.12 ubuntu was: ________________ tests/unit/local_feast_tests/test_e2e_local.py ________________
[gw6] linux -- Python 3.12.13 /home/runner/work/feast/feast/.venv/bin/python3
worker 'gw6' crashed while running 'tests/unit/local_feast_tests/test_e2e_local.py::test_e2e_local'
=============================== warnings summary ==============================Maybe re-running would resolve? |
Sorry, something went wrong.
Hide details
View details
franciscojavierarceo
merged commit
franciscojavierarceo
merged commit 212504b
into
feast-dev:master
Apr 15, 2026
1 of 2 checks passed
ntkathole
pushed a commit
to korbonits/feast
that referenced
this pull request
Apr 22, 2026
Adds three cases to test_retrieve_online_milvus_documents that exercise code paths adjacent to recent MilvusOnlineStore bug fixes (feast-dev#6275): 1. Empty-store query before any writes — verifies the v2 retrieval path returns 0 rows instead of raising when the collection exists but is empty. 2. Oversized top_k (top_k=5 on a 3-row dataset) — verifies the hit-parsing loop handles a result set smaller than the requested top_k and returns all available rows. 3. Cosine-metric variant via a second FeatureView with vector_search_metric="COSINE" — verifies the cosine index path end to end (write, index creation, retrieval). No new imports; all types were already imported in the module. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>
ntkathole
pushed a commit
that referenced
this pull request
Apr 22, 2026
Adds three cases to test_retrieve_online_milvus_documents that exercise code paths adjacent to recent MilvusOnlineStore bug fixes (#6275): 1. Empty-store query before any writes — verifies the v2 retrieval path returns 0 rows instead of raising when the collection exists but is empty. 2. Oversized top_k (top_k=5 on a 3-row dataset) — verifies the hit-parsing loop handles a result set smaller than the requested top_k and returns all available rows. 3. Cosine-metric variant via a second FeatureView with vector_search_metric="COSINE" — verifies the cosine index path end to end (write, index creation, retrieval). No new imports; all types were already imported in the module. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Alex Korbonits <alex@korbonits.com>
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
Reviewers
3 participants
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.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.
What does this PR do?
Fixes five bugs in
sdk/python/feast/infra/online_stores/milvus_online_store/milvus.pyand adds regression tests for three of them.1.
update()overwrites the collection cache (_collections) instead of updating it_get_or_create_collection()already updatesself._collections[collection_name]as a side effect and returns that entry. The assignment inupdate()replaced the entire_collectionsdict with the rawdescribe_collection()dict for the last table, corrupting all subsequent cache lookups for any other collection.Fix: drop the assignment; the side effect in
_get_or_create_collection()is sufficient.2.
plan()raisesNotImplementedErrorinstead of returning[]The
OnlineStorebase class default isreturn []. RaisingNotImplementedErrorcausedfeast planto crash for any project using the Milvus store.3.
online_read()has an extrafull_feature_namesparameter not in the base classThe parameter was unused in the method body and violates the
OnlineStoreinterface contract (online_readtakesrequested_featuresas the last argument).4.
retrieve_online_documents_v2()crashes withTypeErrorwhen the composite key is absent from a hit5.
_connect()usesprint()instead ofloggingConnection messages now use
logger.info()so they respect the application's logging configuration instead of unconditionally writing to stdout.Tests
Three regression tests added in
tests/unit/online_store/test_online_retrieval.py:test_milvus_update_preserves_collection_cache_collectionscache corruption inupdate()test_milvus_plan_returns_empty_listplan()raisingNotImplementedErrortest_milvus_retrieve_online_documents_v2_missing_entity_keyTypeErrorfrombytes.fromhex(None)Full Milvus unit test results:
ruff checkpasses with no errors.🤖 Generated with Claude Code