fix: Resolve write_to_offline_store feature view with a single registry lookup - #6696
fix: Resolve write_to_offline_store feature view with a single registry lookup#6696adarshsm wants to merge 1 commit into
Conversation
…ry lookup write_to_offline_store resolved the feature view with a try/except chain that called get_stream_feature_view, then get_feature_view, then get_label_view in turn. A plain FeatureView -- the common case -- never matches the first lookup, so it was always issued, failed with FeatureViewNotFoundException, and discarded before the lookup that could succeed ran. On a RemoteRegistry each attempt is a gRPC round-trip to the registry server (no client-side cache), so every batch on this per-batch write path paid one or more guaranteed-miss RPCs before the real one. allow_registry_cache=True, the default, does not avoid it on a remote registry. Resolve the feature view with a single registry.get_any_feature_view lookup instead -- the unified accessor added in feast-dev#4235 for exactly the case where a caller holds only a name. It covers FeatureView, StreamFeatureView, OnDemandFeatureView, and LabelView, so it is a behaviour-preserving replacement for the chain while collapsing up to three registry lookups into one. Fixes feast-dev#6671. Signed-off-by: adarshsm <[email protected]>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6696 +/- ##
==========================================
+ Coverage 46.44% 46.79% +0.34%
==========================================
Files 414 414
Lines 50134 50185 +51
Branches 7173 7181 +8
==========================================
+ Hits 23285 23483 +198
+ Misses 25212 25059 -153
- Partials 1637 1643 +6
... and 27 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
franciscojavierarceo
left a comment
There was a problem hiding this comment.
The unified get_any_feature_view lookup preserves the supported feature-view types while removing guaranteed-miss RPCs on the remote-registry write path, and the regression asserts the single lookup. I found no blocking issue in the remote diff.
What this does
FeatureStore.write_to_offline_storeresolved the feature view with a try/except chain (sdk/python/feast/feature_store.py):For a plain
FeatureView— the common case — the first lookup can never succeed. It is issued, raisesFeatureViewNotFoundException, and is discarded, and only then does the lookup that can succeed run. ALabelViewpays two failed lookups before the third.Every getter forwards
allow_registry_cacheas the registry'sallow_cache. On aCachingRegistrythe failed attempts come from the cached proto and cost little. On aRemoteRegistryeach is a gRPC round-trip to the registry server (it keeps no client-side cache and forwardsallow_cacheto the server as a request field). This sits on a per-batch write path, so a backfill pays a guaranteed-miss registry RPC before the real one on every batch, andallow_registry_cache=True(the default) does not avoid it.The fix
Resolve the feature view with a single
registry.get_any_feature_view(name, project, allow_cache=...)lookup — the unified accessor added in #4235 for exactly the case where a caller holds only a name. It coversFeatureView,StreamFeatureView,OnDemandFeatureView, andLabelView, so it is a behaviour-preserving replacement for the chain while collapsing up to three registry lookups into one.#4235introducedget_any_feature_viewto address this same three-getters asymmetry inBaseRegistryand was closed as completed, but this call site was never migrated — it still performed the try/except chain, and a third branch (get_label_view) has been added since.Testing
Added
test_write_to_offline_store_resolves_feature_view_with_single_lookup(sdk/python/tests/unit/test_unit_feature_store.py): it asserts exactly oneget_any_feature_viewcall and that none of the legacy per-type getters fire. The test fails againstmaster(the chain callsget_stream_feature_view, notget_any_feature_view) and passes with this change.ruff check/ruff formatclean on both files;mypyclean onfeature_store.py.Fixes #6671.