fix: Provision online store infra for remote feast apply - #6772
Open
RishabhCodezZz wants to merge 4 commits into
Open
fix: Provision online store infra for remote feast apply#6772RishabhCodezZz wants to merge 4 commits into
RishabhCodezZz wants to merge 4 commits into
Conversation
Applying a new feature view through a client whose online store is `type: remote` leaves it unwritable: the feature view reaches the registry, but no table is provisioned server-side, so the write that `feast materialize` performs returns HTTP 500 with `no such table`. The existing test_remote_online_store_read_write does apply a feature view through a remote client and write to it, but it applies `driver_hourly_stats` -- a name the server already provisioned during its own `feast apply` at setup. The table exists before the client touches it, which masks the bug. This test applies a feature view the server has never seen. The assertion is on the user-visible contract (a feature view applied through a remote online store must be writable) rather than on any particular mechanism, so it holds regardless of how the fix is implemented. Refs feast-dev#6693 Signed-off-by: Rishabh <158596025+RishabhCodezZz@users.noreply.github.com>
`feast apply` against `online_store: type: remote` registered feature views but never created their tables, so `feast materialize` failed with HTTP 500 and `no such table`. `RemoteOnlineStore.update()` and `.teardown()` were both `pass`, and the feature server never calls `update_infra()` on its own -- it only refreshes the registry. Since table DDL lives exclusively in the concrete online store's `update()`, this affected any online store behind `type: remote`, not just SQLite. RemoteOnlineStore.update()/teardown() now call two new feature server endpoints, /update-infra and /teardown-infra, which run update_infra() and teardown_infra() against the server's real online store. The objects travel in the request body as base64-encoded protos rather than being looked up server-side by name, because FeatureStore.apply() calls update_infra() *before* registry.commit() -- at that point the server cannot see them yet, and with a file registry they are not even on disk. Authorization reuses the existing CREATE and DELETE actions, so no new permission surface is introduced. Fixes feast-dev#6693 Signed-off-by: Rishabh <158596025+RishabhCodezZz@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a remote-mode infrastructure provisioning gap in Feast: when online_store.type: remote, a client-side feast apply previously registered objects but did not provision/drop the backing online-store tables on the feature server, causing later writes/materialization to fail.
Changes:
- Implement
RemoteOnlineStore.update()/teardown()to call feature server endpoints to provision and tear down online-store infrastructure. - Add
POST /update-infraandPOST /teardown-infraendpoints to the Python feature server to run provider infra operations server-side. - Add unit + integration regression coverage and document the new endpoints.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/python/feast/infra/online_stores/remote.py | Adds infra object (de)serialization and forwards update/teardown to feature server endpoints. |
| sdk/python/feast/feature_server.py | Introduces /update-infra and /teardown-infra endpoints that invoke provider infra operations. |
| sdk/python/tests/unit/infra/online_store/test_remote_online_store.py | Adds unit tests for serialization and update/teardown request/response handling. |
| sdk/python/tests/integration/online_store/test_remote_online_store_provisioning.py | Adds end-to-end regression test ensuring remote apply provisions a previously-unknown FV table. |
| docs/reference/feature-servers/python-feature-server.md | Documents the new infra endpoints in the feature server API table. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+99
to
+104
| type_name = type(obj).__name__ | ||
| if type_name not in _infra_object_types(): | ||
| raise ValueError( | ||
| f"Cannot send {type_name} to the remote online store; " | ||
| f"expected one of {sorted(_infra_object_types())}" | ||
| ) |
Comment on lines
+977
to
+980
| for table in tables_to_keep: | ||
| assert_permissions(resource=table, actions=[AuthzedAction.CREATE]) | ||
| for table in tables_to_delete: | ||
| assert_permissions(resource=table, actions=[AuthzedAction.DELETE]) |
Comment on lines
+119
to
+122
| proc.kill() | ||
| proc.wait(timeout=30) | ||
| log_file.close() | ||
|
|
Comment on lines
+1002
to
+1004
| for table in tables: | ||
| assert_permissions(resource=table, actions=[AuthzedAction.DELETE]) | ||
|
|
Review follow-ups on the remote online store provisioning endpoints. /update-infra and /teardown-infra asserted permissions on the feature views in the request body but not on the entities, so a caller could provision or drop entity infrastructure without an authorization check. Both endpoints now apply the same CREATE and DELETE actions to entities that they already apply to tables. _infra_object_types() re-ran its five lazy imports on every call, and encode_infra_object() invoked it twice per object across four lists. It is now cached and read once per call. The integration test proved that apply() provisions a table but never exercised the teardown path, and both of its fixtures leaked their temp directories. It now asserts against the server's own SQLite file on both sides of the lifecycle -- the table exists after apply and is gone after teardown, which covers /update-infra and /teardown-infra end to end -- and removes its temp directories on the way out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Rishabh <158596025+RishabhCodezZz@users.noreply.github.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
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 this PR does / why we need it:
feast applyagainstonline_store: type: remoteregisters feature views in the registry but never creates their backing tables. A subsequentfeast materializefails with HTTP 500 (sqlite3.OperationalError: no such table).Root cause:
RemoteOnlineStore.update()andteardown()were bothpass— no provisioning call was ever forwarded to the server. The feature server refreshes the registry viaasync_refresh()but never callsupdate_infra(). Since table DDL lives exclusively in each concrete store'supdate()method, this affects every online store behindtype: remote, not just SQLite.Fix:
RemoteOnlineStore.update()andteardown()now call two new feature server endpoints (/update-infraand/teardown-infra) that run the real provisioning against the server's online store.Objects travel in the request body as base64-encoded protobufs rather than being looked up by name, because
FeatureStore.apply()callsupdate_infra()beforeregistry.commit()— at that point the server can't see them yet. Authorization reuses existingCREATE/DELETEactions (no new permissions).Changes:
feast/infra/online_stores/remote.pyupdate()andteardown(); added encode/decode helpers and HTTP functionsfeast/feature_server.pyPOST /update-infraandPOST /teardown-infraendpoints with request modelstests/integration/.../test_remote_online_store_provisioning.pytests/unit/.../test_remote_online_store.pydocs/reference/feature-servers/python-feature-server.mdWhy the existing test didn't catch this:
test_remote_online_store_read_writeappliesdriver_hourly_stats, which the server already provisioned during its own setup. The table exists before the remote client touches it. The new test applies a name the server has never seen, and asserts against the server's own SQLite file rather than inferring success from a write returning 200.Notes for reviewers
Two deliberate choices worth calling out, both easy to change if you would rather go the other way:
1. The endpoints call
provider.update_infra()/provider.teardown_infra(), not just the online store.PassthroughProvider.update_infra()also invokesbatch_engine.update(). For the default engine that is a no-op, but for engines that provision real resources this means a remotefeast applynow provisions batch-engine infra server-side too. I think that is right — in remote mode the server is the side that actually holds the infra config — but it is broader than the "online store" framing in the title, so say the word if you would prefer it narrowed toonline_store.update().2. Version skew is now a loud failure instead of a silent one. A client on this version talking to a feature server that predates these endpoints gets a 404, which surfaces as a
RuntimeErrorout offeast apply. Previously the call silently did nothing and the problem only showed up later asno such tableduring materialization. Failing at apply time seems strictly better, but it is a behaviour change for mixed-version deployments and probably deserves a release note.Which issue(s) this PR fixes:
Fixes #6693
Checks
git commit -s)Testing Strategy
Misc
Design choice approved by @yuan1j in #6693 — Option A (client-driven, synchronous provisioning via new endpoints).
The first two commits are ordered red-then-green: the failing test lands first, the fix second, so the git history itself proves the test catches the bug. The third commit addresses review feedback — entity-level authorization on both endpoints, caching the lazily-built type map, and extending the integration test to assert the table is present after apply and gone after teardown, so both new endpoints are covered end to end.