fix: Use matching proto class per feature view list in SqliteOnlineStore.plan() - #6659
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6659 +/- ##
==========================================
+ Coverage 45.93% 45.99% +0.05%
==========================================
Files 414 414
Lines 50006 50037 +31
Branches 7147 7147
==========================================
+ Hits 22972 23014 +42
+ Misses 25423 25412 -11
Partials 1611 1611
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
02e0617 to
47550c8
Compare
47550c8 to
86cf676
Compare
|
The `unit-test-go` failure looks unrelated to this change (only Python files are touched here): `open .../go/internal/test/feature_repo/data/registry.db: no such file or directory` in `TestNewFeatureStore`. That same job passed on the base commit this branch is built from (104ad10), so it looks like environmental flakiness in that job rather than something introduced by this PR. Happy to look further if it's actually related -- let me know. |
franciscojavierarceo
left a comment
There was a problem hiding this comment.
Using the matching class for each proto list fixes the typeguard failure while preserving the table-ID path, and the new tests cover stream-only and mixed registries. I found no blocking issue in the remote diff.
DynamoDBOnlineStore has no plan() override, so it inherits OnlineStore.plan()'s no-op default: `feast plan` never reports any DynamoDB infrastructure changes, unlike SqliteOnlineStore and MilvusOnlineStore which both implement it. Adds DynamoDBTable (InfraObject) and DynamoDBOnlineStore.plan(), using the InfraObject proto's CustomInfra field (protos/feast/core/InfraObject.proto), which exists specifically so online stores can add InfraObject support without changes to the core proto -- no other in-tree store uses it yet. Note this only affects `feast plan`'s reporting: `feast apply`'s diff-based path (FeatureStore._should_use_plan(), which would call InfraObject.update()/teardown()) is gated to the local/sqlite provider only, so DynamoDBOnlineStore.update()/teardown() -- which already perform the real table creation/deletion -- are unaffected. Uses the corrected feature-view-list pattern (see #6658 / #6659): FeatureView.from_proto() and StreamFeatureView.from_proto() applied to their respective proto lists, not one applied to both. Signed-off-by: Carlos Sánchez <carlos.sancheza@cabify.com>
…ore.plan() FeatureView.from_proto() was applied uniformly to both feature_views and stream_feature_views. FeatureView is @TypeChecked and its from_proto() parameter is annotated as FeatureViewProto, so passing a StreamFeatureViewProto raises typeguard.TypeCheckError at runtime. Use StreamFeatureView.from_proto() for the stream_feature_views list instead, matching the actual proto type of each entry. Fixes feast-dev#6658 Signed-off-by: Carlos Sánchez <infsaarc@gmail.com>
86cf676 to
d972891
Compare
What this PR does / why we need it
Fixes #6658.
SqliteOnlineStore.plan()appliesFeatureView.from_proto()uniformly to bothdesired_registry_proto.feature_viewsanddesired_registry_proto.stream_feature_views.FeatureViewis@typechecked, andfrom_proto's parameter is annotated asFeatureViewProto, so passing aStreamFeatureViewProto(from thestream_feature_viewslist) raisestypeguard.TypeCheckErrorat runtime --plan()(and thereforefeast plan) fails outright on any repo with at least oneStreamFeatureViewregistered.This uses the matching class per list instead:
_table_id()/compute_table_id()/compute_versioned_name()only touch.name,.projection, and.current_version_number(the latter two viagetattrwith defaults), so aStreamFeatureViewinstance works the same as aFeatureViewinstance there -- no other change needed.We ran into this adding
plan()/InfraObjectsupport to a custom online store and initially copied this exact pattern fromsqlite.pyas the only in-tree reference implementation ofplan().Which issue(s) this PR fixes
Fixes #6658
Does this PR introduce a user-facing change?
Test plan
sdk/python/tests/unit/infra/online_store/test_sqlite_plan.pywith two cases: a registry with only aStreamFeatureView, and one with a batchFeatureViewand aStreamFeatureViewtogether.masterbefore this change, and pass after.sdk/python/tests/unit/infra/online_store/directory (206 tests) -- no regressions.ruff checkandruff format --checkpass on the changed files.