GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat: Add remote materialization support via feature server by aniketpalu · Pull Request #6590 · feast-dev/feast Conversation
|
| materialize_mode: Literal["local", "remote"] = "local" | ||
| """When 'remote', store.materialize() delegates to the feature server's | ||
| async endpoint instead of running the batch engine locally.""" |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think we dont need to provide the materialize_mode config. We should strictly feature server take care of materialization delegation to compute engine. Leaving no choice to do materialization on client side.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A cleaner approach might be: a RemoteComputeEngine (like RemoteOnlineStore/RemoteOfflineStore) that POSTs to the existing /materialize endpoint, returning a job handle with a dedicated /materialize-status/{job_id} endpoint - keeping the materialization delegation at the engine layer rather than the FeatureStore layer.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
if you mean something like
batch_engine:
type: remote.engine
path: https://feast-online:443 # same host as online
cert: /var/run/secrets/.../service-ca.crt
I thought about this before choosing current approach. I see two points which made me not take this decision.
My proposed approach is
store.materialize(remote=True, wait=False) --> pre-process check & return 202 Accepted response
store.poll_materialization() --> returns status
This approach has
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@aniketpalu Ok, so, I think this PR is mixing three orthogonal concerns: async execution, remote delegation, and job tracking.
Remote delegation already works today - POST /materialize runs materialization server-side. A client with online_store: remote can call this endpoint directly. As @jyejare mentioned, the feature server should strictly handle materialization - no per-call remote=True flag needed; the deployment topology already decides this.
Async execution - The real gap is that long-running engines (SparkApplication) block the HTTP caller. The existing /materialize endpoint just needs an async mode (?async=true → 202 with job_id) rather than entirely new /materialize-async endpoints. This serves both UI on-demand requests and SDK wait=False use cases, while CronJob continues using the existing sync path unchanged.
Job tracking - Polling FeatureViewState from the registry is fragile (race conditions, no job_id, ambiguous failure, breaks FeatureView.eq). If we need job tracking, it should be a dedicated MaterializationJob entity in the registry with proper RPCs - not piggybacking on FV metadata. For SparkApplication, the SparkApp CR status on K8s is already the source of truth.
May be we can scope this PR to just remote delegation and async execution ? ?async=true on the existing /materialize endpoint (fire-and-forget, returns 202. Completion is already observable via existing mechanisms like materialization_intervals on the FV, SparkApp CR status) + SDK support for async with remote delegation.
Job tracking with dedicated status endpoints / registry entities can come as a separate effort.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Remote delegation already works today - POST /materialize runs materialization server-side. A client with online_store: remote can call this endpoint directly. As @jyejare mentioned, the feature server should strictly handle materialization - no per-call remote=True flag needed; the deployment topology already decides this.
Async execution - The real gap is that long-running engines (SparkApplication) block the HTTP caller. The existing /materialize endpoint just needs an async mode (?async=true → 202 with job_id) rather than entirely new /materialize-async endpoints. This serves both UI on-demand requests and SDK wait=False use cases, while CronJob continues using the existing sync path unchanged.
agree on both point
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
To keep the changes and commit history clean, I have raised separate PR. Would love your feedback on that. #6649
Sorry, something went wrong.
There was an error while loading. Please reload this page.
apply_materialization() in SQL and Snowflake registries did not transition FeatureViewState like the file-based registry does. This caused remote materialization polling (and any state-based monitoring) to never see completion when using these registry backends. Signed-off-by: Aniket Paluskar <[email protected]>
- Pass online_store.cert as verify= on the materialize-async HTTP call (same server hosts both online store and materialize endpoints) - Set FV state to AVAILABLE_ONLINE in SQL/Snowflake registry apply_materialization() to match file-based registry behavior Signed-off-by: Aniket Paluskar <[email protected]>
Opt in with materialize(..., remote=True); URL/TLS from online_store. Add wait=False and poll_materialization(); set MATERIALIZING before 202 and reset to GENERATED on async failure; include state in FeatureView.__eq__.
Keep feast-dev#6550 batching in materialize loops; skip FV state transition when _force_local so async /materialize-async (already MATERIALIZING) works. Co-authored-by: Cursor <[email protected]>
Keep feast-dev#6550 batching; skip FV state transition when _force_local so /materialize-async (already MATERIALIZING) works with SparkApplication. Co-authored-by: Cursor <[email protected]>
Keep feast-dev#6550 batching; skip FV state transition when _force_local so /materialize-async (already MATERIALIZING) works with SparkApplication. Co-authored-by: Cursor <[email protected]>
Bring in feast-dev#6550 SparkApplication (supports_batch / _materialize_fvs_batch). Resolve feature_store.py by keeping master batch structure and threading _force_local through transition/rollback/batch helpers so async remote-mat does not double-set MATERIALIZING or wrongly roll back state. Co-authored-by: Cursor <[email protected]>
There was an error while loading. Please reload this page.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## master #6590 +/- ## ========================================== - Coverage 45.93% 45.89% -0.05% ========================================== Files 414 414 Lines 50006 50077 +71 Branches 7147 7157 +10 ========================================== + Hits 22972 22981 +9 - Misses 25423 25485 +62 Partials 1611 1611
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Refresh with current feast-dev#6590 tip (includes master merge with feast-dev#6550). Keep spark_application Dockerfile postgres extra for E2E. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This PR adds remote materialization support via feature server, allowing clients to delegate materialization to a remote server instead of running it locally. The implementation includes async endpoints in the feature server, client-side remote delegation logic, status polling mechanisms, and comprehensive test coverage. The feature is well-architected with proper error handling and state management.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| def _authorize_materialize_views( | ||
| request_feature_views: Optional[List[str]], | ||
| ) -> List[str]: | ||
| """Shared authz + FV name resolution for async materialize endpoints.""" | ||
| feature_views_to_materialize = store._get_feature_views_to_materialize( | ||
| request_feature_views | ||
| ) | ||
| for fv in feature_views_to_materialize: | ||
| assert_permissions( | ||
| resource=fv, | ||
| actions=[AuthzedAction.WRITE_ONLINE], | ||
| ) | ||
| return [fv.name for fv in feature_views_to_materialize] | ||
|
|
||
| def _parse_materialize_timestamps(request: MaterializeRequest): | ||
| """Parse and validate timestamps from a materialize request.""" |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
[Critical] Function defined inside endpoint handler scope
The function _authorize_materialize_views is defined inside the endpoint handler scope but references store from the outer scope. This creates a closure that captures the store variable, which could lead to unexpected behavior if the store changes or if this function is called in different contexts. This pattern violates the principle of explicit dependencies and makes testing difficult.
Suggested:
| def _authorize_materialize_views( | |
| request_feature_views: Optional[List[str]], | |
| ) -> List[str]: | |
| """Shared authz + FV name resolution for async materialize endpoints.""" | |
| feature_views_to_materialize = store._get_feature_views_to_materialize( | |
| request_feature_views | |
| ) | |
| for fv in feature_views_to_materialize: | |
| assert_permissions( | |
| resource=fv, | |
| actions=[AuthzedAction.WRITE_ONLINE], | |
| ) | |
| return [fv.name for fv in feature_views_to_materialize] | |
| def _parse_materialize_timestamps(request: MaterializeRequest): | |
| """Parse and validate timestamps from a materialize request.""" | |
| def _authorize_materialize_views( | |
| store: FeatureStore, request_feature_views: Optional[List[str]] | |
| ) -> List[str]: | |
| """Shared authz + FV name resolution for async materialize endpoints.""" | |
| feature_views_to_materialize = store._get_feature_views_to_materialize( | |
| request_feature_views | |
| ) | |
| for fv in feature_views_to_materialize: | |
| assert_permissions( | |
| resource=fv, | |
| actions=[AuthzedAction.WRITE_ONLINE], | |
| ) | |
| return [fv.name for fv in feature_views_to_materialize] |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| def _check_already_materializing(fv_names: List[str]) -> Optional[JSONResponse]: | ||
| """Return 409 if any requested FV is already materializing.""" | ||
| for fv_name in fv_names: | ||
| try: | ||
| fv = store.registry.get_feature_view( | ||
| fv_name, store.project, allow_cache=False | ||
| ) | ||
| if getattr(fv, "state", None) == FeatureViewState.MATERIALIZING: | ||
| return JSONResponse( | ||
| status_code=409, | ||
| content={ | ||
| "error": f"Feature view '{fv_name}' is already materializing" | ||
| }, | ||
| ) | ||
| except Exception: | ||
| pass | ||
| return None |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
[Critical] Function defined inside endpoint handler with closure dependencies
Similar to the previous issue, _check_already_materializing and other helper functions are defined inside the endpoint handler and capture variables from outer scope. This makes the code harder to test and reason about. These functions should be extracted as proper module-level functions with explicit parameters.
Suggested:
| def _check_already_materializing(fv_names: List[str]) -> Optional[JSONResponse]: | |
| """Return 409 if any requested FV is already materializing.""" | |
| for fv_name in fv_names: | |
| try: | |
| fv = store.registry.get_feature_view( | |
| fv_name, store.project, allow_cache=False | |
| ) | |
| if getattr(fv, "state", None) == FeatureViewState.MATERIALIZING: | |
| return JSONResponse( | |
| status_code=409, | |
| content={ | |
| "error": f"Feature view '{fv_name}' is already materializing" | |
| }, | |
| ) | |
| except Exception: | |
| pass | |
| return None | |
| def _check_already_materializing( | |
| store: FeatureStore, fv_names: List[str] | |
| ) -> Optional[JSONResponse]: | |
| """Return 409 if any requested FV is already materializing.""" | |
| for fv_name in fv_names: | |
| try: | |
| fv = store.registry.get_feature_view( | |
| fv_name, store.project, allow_cache=False | |
| ) | |
| if getattr(fv, "state", None) == FeatureViewState.MATERIALIZING: | |
| return JSONResponse( | |
| status_code=409, | |
| content={ | |
| "error": f"Feature view '{fv_name}' is already materializing" | |
| }, | |
| ) | |
| except Exception: | |
| pass | |
| return None |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| @app.post("/materialize-async", dependencies=[Depends(inject_user_details)]) | ||
| async def materialize_async(request: MaterializeRequest) -> JSONResponse: | ||
| with feast_metrics.track_request_latency("/materialize-async"): | ||
| fv_names = _authorize_materialize_views(request.feature_views) | ||
| start_date, end_date = _parse_materialize_timestamps(request) | ||
|
|
||
| conflict = _check_already_materializing(fv_names) | ||
| if conflict: | ||
| return conflict | ||
|
|
||
| _update_fv_state(fv_names, FeatureViewState.MATERIALIZING) | ||
|
|
||
| def _run_materialize(): | ||
| try: | ||
| store.materialize( |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
[Warning] Materialize async endpoint lacks proper error handling
The /materialize-async endpoint catches all exceptions in the background thread but only logs them. If the materialization fails during the initial setup (before the background thread), the state won't be properly reset. The endpoint should have a try-catch around the initial setup and reset states appropriately.
Suggested:
| @app.post("/materialize-async", dependencies=[Depends(inject_user_details)]) | |
| async def materialize_async(request: MaterializeRequest) -> JSONResponse: | |
| with feast_metrics.track_request_latency("/materialize-async"): | |
| fv_names = _authorize_materialize_views(request.feature_views) | |
| start_date, end_date = _parse_materialize_timestamps(request) | |
| conflict = _check_already_materializing(fv_names) | |
| if conflict: | |
| return conflict | |
| _update_fv_state(fv_names, FeatureViewState.MATERIALIZING) | |
| def _run_materialize(): | |
| try: | |
| store.materialize( | |
| def _run_materialize(): | |
| try: | |
| store.materialize( | |
| start_date, | |
| end_date, | |
| request.feature_views, | |
| disable_event_timestamp=request.disable_event_timestamp, | |
| full_feature_names=request.full_feature_names, | |
| version=request.version, | |
| _force_local=True, | |
| ) | |
| # Set state to AVAILABLE_ONLINE on success | |
| _update_fv_state(fv_names, FeatureViewState.AVAILABLE_ONLINE) | |
| except Exception as e: | |
| logger.error( | |
| f"Async materialization failed for {fv_names}: {e}", | |
| exc_info=True, | |
| ) | |
| _update_fv_state(fv_names, FeatureViewState.GENERATED) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| feature_view, | ||
| already_transitioned: list, | ||
| previous_states: dict, | ||
| _force_local: bool = False, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
[Warning] Empty return in state transition function breaks existing logic
Adding an early return when _force_local=True means the previous_states[feature_view.name] = previous_state line is never reached, which could break rollback logic elsewhere. The function should still record the previous state even when skipping the transition.
Suggested:
| feature_view, | |
| already_transitioned: list, | |
| previous_states: dict, | |
| _force_local: bool = False, | |
| Skip when _force_local: the async handler already set MATERIALIZING. | |
| """ | |
| previous_state = getattr(feature_view, "state", None) | |
| previous_states[feature_view.name] = previous_state | |
| if _force_local: | |
| return # Skip state transition but preserve previous_state recording |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return FVMaterializationStatus.FAILED | ||
| return FVMaterializationStatus.PENDING | ||
| return FVMaterializationStatus.PENDING | ||
|
|
||
| def poll_materialization( | ||
| self, | ||
| feature_views: Optional[List[str]] = None, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
[Warning] State mapping logic could be more explicit
The _fv_state_to_materialization_status function uses a mutable seen_materializing set that's passed by reference. This side-effect pattern makes the function harder to reason about and test. Consider returning a tuple or using a more functional approach.
Suggested:
| return FVMaterializationStatus.FAILED | |
| return FVMaterializationStatus.PENDING | |
| return FVMaterializationStatus.PENDING | |
| def poll_materialization( | |
| self, | |
| feature_views: Optional[List[str]] = None, | |
| def _fv_state_to_materialization_status( | |
| self, state, *, seen_materializing: Optional[set] = None, fv_name: str = "" | |
| ) -> Tuple[FVMaterializationStatus, bool]: | |
| """Map FeatureViewState to FVMaterializationStatus. | |
| Returns: | |
| Tuple of (status, was_materializing) where was_materializing indicates | |
| if this FV was seen in MATERIALIZING state. | |
| """ | |
| from feast.materialization_status import FVMaterializationStatus | |
| if state == FeatureViewState.AVAILABLE_ONLINE: | |
| return FVMaterializationStatus.SUCCEEDED, False | |
| if state == FeatureViewState.MATERIALIZING: | |
| if seen_materializing is not None: | |
| seen_materializing.add(fv_name) | |
| return FVMaterializationStatus.RUNNING, True | |
| if state == FeatureViewState.GENERATED: | |
| was_materializing = seen_materializing is not None and fv_name in seen_materializing | |
| status = FVMaterializationStatus.FAILED if was_materializing else FVMaterializationStatus.PENDING | |
| return status, was_materializing | |
| return FVMaterializationStatus.PENDING, False |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| end_date: datetime, | ||
| feature_views: Optional[List[str]] = None, | ||
| disable_event_timestamp: bool = False, | ||
| full_feature_names: bool = False, | ||
| version: Optional[str] = None, | ||
| timeout: float = 3600.0, | ||
| poll_interval: float = 5.0, | ||
| wait: bool = True, | ||
| ) -> None: | ||
| """Delegate materialization to the feature server, poll via registry.""" | ||
| fv_names = feature_views or [ |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
[Warning] HTTP session not properly closed in all paths
The HTTP session is closed in a finally block, but if an exception occurs during session creation or configuration, the session might never be assigned, leading to a NameError. The session creation and closure should be more robust.
Suggested:
| end_date: datetime, | |
| feature_views: Optional[List[str]] = None, | |
| disable_event_timestamp: bool = False, | |
| full_feature_names: bool = False, | |
| version: Optional[str] = None, | |
| timeout: float = 3600.0, | |
| poll_interval: float = 5.0, | |
| wait: bool = True, | |
| ) -> None: | |
| """Delegate materialization to the feature server, poll via registry.""" | |
| fv_names = feature_views or [ | |
| session = None | |
| try: | |
| session = self._get_remote_http_session() | |
| post_kwargs: dict = { | |
| "json": payload, | |
| "timeout": 30, | |
| } | |
| cert = getattr(self.config.online_store, "cert", None) | |
| if cert: | |
| post_kwargs["verify"] = cert | |
| resp = session.post(f"{server_url}{endpoint}", **post_kwargs) | |
| resp.raise_for_status() | |
| except Exception as e: | |
| raise Exception( | |
| f"Failed to trigger remote materialization at " | |
| f"{server_url}{endpoint}: {e}" | |
| ) from e | |
| finally: | |
| if session is not None: | |
| session.close() |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@aniketpalu Please fix DCO check for incorrect signoff. Also please update docs for remote materialization support. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…alize-incremental When async=true, endpoints fire off materialization in a background thread and immediately return 202 Accepted with the list of resolved feature views. Concurrent requests for an already-MATERIALIZING FV are rejected with 409. Without ?async=true, behavior is unchanged (sync, blocking). Four module-level helpers extracted for testability: - _authorize_materialize_views: resolve + authz - _check_already_materializing: 409 guard - _update_fv_state: registry state transitions - _parse_materialize_timestamps: input validation Also fix: previous_states dict was never populated in _transition_fv_to_materializing, so rollback could never restore states. Signed-off-by: Aniket Paluskar <[email protected]>
_check_already_materializing now collects every conflicting feature view instead of returning on the first match, and includes both a clear error message and a feature_views list in the 409 body. Signed-off-by: Aniket Paluskar <[email protected]>
When ?async=true&force=true, the 409 concurrency guard is skipped, allowing operators to re-trigger materialization for FVs stuck in MATERIALIZING state (e.g. after a crashed SparkApplication pod). force=true without async=true has no effect. Also updates the 409 error message to hint at ?force=true. Signed-off-by: Aniket Paluskar <[email protected]>
620c103 to
0be2b53
Compare
0be2b53 to
620c103
Compare
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.