fix: Merge shared ODFV source projections in feature resolution - #6622
fix: Merge shared ODFV source projections in feature resolution#6622sanskar-singh-2403 wants to merge 1 commit into
Conversation
072cf48 to
621c1b1
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6622 +/- ##
==========================================
+ Coverage 45.88% 45.89% +0.01%
==========================================
Files 414 414
Lines 49950 49967 +17
Branches 7140 7142 +2
==========================================
+ Hits 22918 22933 +15
- Misses 25431 25432 +1
- Partials 1601 1602 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| fvs_to_use.append( | ||
| source_fv.with_projection(copy.copy(source_projection)) | ||
| projection_key = source_projection.name_to_use() | ||
| existing_source_fv = fvs_by_projection_key.get(projection_key) |
There was a problem hiding this comment.
fvs_by_projection_key is only populated for ODFV sources. regular FeatureView is appended but not indexed, so ["src_fv:a", "odfv_b:b_out"] creates duplicate src_fv entries and later raises KeyError: Feature a not found in projection src_fv.
Could both paths use the same append-or-merge helper keyed by projection.name_to_use()? I tested that approach in both orders, it produces one merged entry and both requests passed.
There was a problem hiding this comment.
Good catch @addenergyx , you're right. fvs_by_projection_key was only written in the ODFV branch, so a regular FeatureView got appended but never indexed. ["src_fv:a", "odfv_b:b_out"] then appended a second, partial src_fv projection and blew up later with KeyError: Feature a not found in projection src_fv.
I pulled the logic into a single append-or-merge helper keyed by projection.name_to_use() and routed both paths (regular FV and ODFV source) through it, as you suggested. Added two regression tests covering the mixed regular-FV + ODFV-source case in both orders; confirmed they fail on the old code and pass with the fix. Full test_utils.py is green.
There was a problem hiding this comment.
Perfect! Thanks for the fix @sanskar-singh-2403
5d68811 to
d93d559
Compare
|
@franciscojavierarceo @ntkathole requesting your reviews in this, Thanks! |
When several on demand feature views share a source feature view but select different input columns, _get_feature_views_to_use appended the shared source view once per ODFV, each copy carrying only that ODFV's projection. The membership check 'if source_fv not in fvs_to_use' never matched because BaseFeatureView.__eq__ compares projections and the freshly fetched full-projection view never equals a stored narrowed copy. Downstream, _group_feature_refs keys its view index by projection.name_to_use(), so the last duplicate won and the other ODFVs' source projections were silently dropped, producing missing inputs (NaN in pandas mode) during retrieval. Index accumulated source views by projection.name_to_use() and merge projections (union of features, deduped by name) instead of appending a duplicate, so a shared source view resolves to a single entry carrying the union of the required input columns. The merge helper reassigns projection.features to a fresh list rather than mutating in place, because the projections in play are shallow copies (copy.copy) that share their features list with the registry object. Adds TestGetFeatureViewsToUseSharedSource in tests/unit/test_utils.py covering the merged-projection result and order independence. Fixes feast-dev#6621 Signed-off-by: Sanskar Singh <sanskarsinghty1234@gmail.com>
d93d559 to
ea94468
Compare
3 participants
What this does
Fixes #6621.
When several on demand feature views share a source feature view but select different input columns, _get_feature_views_to_use appended the shared source view once per ODFV, each copy carrying only that ODFV's projection. The membership guard if source_fv not in fvs_to_use never matched, because BaseFeatureView.eq compares projections and the freshly-fetched full-projection view never equals an already-stored narrowed copy.
Downstream, _group_feature_refs keys its view index by projection.name_to_use(), so the duplicate entries collide and the last one wins, the other ODFVs' source projections are silently dropped. During get_historical_features this means the losing ODFV's input columns are never fetched, so transforms receive missing inputs (NaN in pandas mode).
The offline retrieval path is masked on 0.62+ by the ref-injection loop from #6140, but the resolution helper still returns duplicate entries with conflicting projections to its other callers (_get_online_request_context, retrieve_online_documents, retrieve_online_documents_v2).
The fix
In _get_feature_views_to_use, index accumulated source views by projection.name_to_use() and, on a repeat hit, merge the projections (union of features, deduped by name) instead of appending a duplicate. A shared source view now resolves to a single entry carrying the union of the required input columns.
The merge helper reassigns projection.features to a fresh list rather than mutating in place, because the projections in play are shallow copies (copy.copy) that share their features list with the registry object.
Tests
Added TestGetFeatureViewsToUseSharedSource in tests/unit/test_utils.py:
Both fail on master (assert 2 == 1 the duplicate entries) and pass with this change. ruff check and ruff format are clean.
Related
#6099, #6140