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
Copy link
Copy Markdown
Sorry, something went wrong.
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>
Copy link
Copy Markdown
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-infra and POST /teardown-infra endpoints 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.
Sorry, something went wrong.
Comment thread
sdk/python/feast/infra/online_stores/remote.py
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 thread
sdk/python/feast/feature_server.py
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 thread
sdk/python/feast/feature_server.py
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 apply against online_store: type: remote registers feature views in the registry but never creates their backing tables. A subsequent feast materialize fails with HTTP 500 (sqlite3.OperationalError: no such table).
Root cause: RemoteOnlineStore.update() and teardown() were both pass — no provisioning call was ever forwarded to the server. The feature server refreshes the registry via async_refresh() but never calls update_infra(). Since table DDL lives exclusively in each concrete store's update() method, this affects every online store behind type: remote, not just SQLite.
Fix: RemoteOnlineStore.update() and teardown() now call two new feature server endpoints (/update-infra and /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() calls update_infra() before registry.commit() — at that point the server can't see them yet. Authorization reuses existing CREATE/DELETE actions (no new permissions).
Changes:
Why the existing test didn't catch this: test_remote_online_store_read_write applies driver_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 invokes batch_engine.update(). For the default engine that is a no-op, but for engines that provision real resources this means a remote feast apply now 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 to online_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 RuntimeError out of feast apply. Previously the call silently did nothing and the problem only showed up later as no such table during 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
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.