fix: Fix five bugs in milvus online store#6275
Open
korbonits wants to merge 6 commits intofeast-dev:masterfrom
Open
fix: Fix five bugs in milvus online store#6275korbonits wants to merge 6 commits intofeast-dev:masterfrom
korbonits wants to merge 6 commits intofeast-dev:masterfrom
Conversation
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>
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? |
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
3 participants
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.
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