feat: Bring Your Own Spark - SparkApplication#6550
Conversation
…merged-spark-e2e
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 <cursoragent@cursor.com>
…merged-spark-e2e
Keep feast-dev#6550 batching; skip FV state transition when _force_local so /materialize-async (already MATERIALIZING) works with SparkApplication. Co-authored-by: Cursor <cursoragent@cursor.com>
…aterialization Adds a new batch compute engine that submits materialization jobs as SparkApplication CRDs via the Kubeflow Spark Operator. One 'feast materialize' call creates one SparkApplication pod that processes all feature views using distributed Spark, rather than running in-process on the Feast server. Key changes: - Refactor materialize()/materialize_incremental() to pass all tasks to the engine in a single batch call instead of looping per feature view. Existing engines are unaffected (base class loops tasks internally via _materialize_one). - Add public get_provider() method on FeatureStore. - New spark_application engine: config, compute, job, driver script, Dockerfile. - 12 unit tests covering config, validation, CR structure, state mapping, timeout, cleanup, and job naming. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
- Pod calls apply_materialization via gRPC after each successful FV, setting state to AVAILABLE_ONLINE. Server reads FV state post-completion to determine per-FV success/failure in batched SparkApplication runs. - registry_address is now mandatory (simplified from complex path heuristic). - Dockerfile rewritten to install feast from source (matches K8s engine pattern). - Unit tests updated: 15/15 pass (3 new tests for _build_per_fv_jobs). - E2E validated: 5 FVs x 9600 rows, 5 executors, all AVAILABLE_ONLINE. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
… stores - Config delivery: Secret → ConfigMap. Operator's ClusterRole already has full ConfigMap CRUD — avoids widening RBAC for Secrets in ODH. Matches KubernetesComputeEngine pattern. - Removed registry_address config field. Pod inherits server's registry config (SQL, Snowflake) directly and writes apply_materialization() to the same database. Eliminates TLS certificate mounting complexity. - Reject file-based offline stores (dask, file, duckdb) and registries (file) at __init__(), same as existing sqlite/faiss online store rejection. SparkApplication pod has ephemeral filesystem. - Dockerfile: added PYTHONPATH/SPARK_HOME for PySpark, added pymysql. - 20/20 unit tests pass. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
9578721 to
7a27217
Compare
Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
|
@aniketpalu lint checks are failing |
jyejare
left a comment
There was a problem hiding this comment.
While the core architecture is solid, there are several critical issues around thread safety, error handling, and resource management that need addressing before merge.
| @@ -424,6 +424,113 @@ def _get_provider(self) -> Provider: | |||
| # TODO: Bake self.repo_path into self.config so that we dont only have one interface to paths | |||
There was a problem hiding this comment.
[Critical] Race condition in state management
The state transition and rollback logic has potential race conditions when multiple materialization jobs run concurrently. The previous_states dictionary and feature view state updates are not atomic, which could lead to inconsistent states or lost rollbacks.
Suggested:
| # TODO: Bake self.repo_path into self.config so that we dont only have one interface to paths | |
| # Use locks or atomic operations for state management | |
| import threading | |
| state_lock = threading.Lock() | |
| with state_lock: | |
| for fv, job in zip(regular_fvs, jobs): | |
| fv_status = job.status() | |
| if fv_status == MaterializationJobStatus.ERROR: | |
| failed_fvs.append(fv) | |
| if first_error is None and job.error(): | |
| first_error = job.error() | |
| else: | |
| succeeded_fvs.append(fv) | |
| if failed_fvs: | |
| self._rollback_fv_states(failed_fvs, previous_states) |
There was a problem hiding this comment.
The threading race condition described isn't present because previous_states is a local variable (line 2551) and all loops (for fv, job in zip(...)) are sequential Python for loops, not threaded. Each materialize() call creates its own dict.
However, reviewing this code path revealed a shared-object bug in our engine's _build_per_fv_jobs. Previously, all succeeded FVs received the same SparkApplicationMaterializationJob reference. During _wait_for_completion, polling sets _error on that object when the SparkApp transitions to FAILED. Later, when feature_store.py calls job.status() for each FV, they all hit the if self._error is not None: return ERROR early-return, even for the FVs that actually succeeded and wrote AVAILABLE_ONLINE to the registry.
| "staging_location is configured but only used for " | ||
| "get_historical_features (not yet supported by this engine). " | ||
| "It will be ignored for materialize operations.", | ||
| stacklevel=2, | ||
| ) | ||
| for i, entry in enumerate(self.env): | ||
| if "name" not in entry: | ||
| raise ValueError( | ||
| f"env[{i}] is missing required 'name' key. " | ||
| f"Each env entry must have at least 'name' and 'value'. Got: {entry}" | ||
| ) |
There was a problem hiding this comment.
[Suggestion] Improve configuration validation
The env validation only checks for 'name' field but doesn't validate that 'value' exists or that the overall structure is correct for Kubernetes environment variables.
Suggested:
| "staging_location is configured but only used for " | |
| "get_historical_features (not yet supported by this engine). " | |
| "It will be ignored for materialize operations.", | |
| stacklevel=2, | |
| ) | |
| for i, entry in enumerate(self.env): | |
| if "name" not in entry: | |
| raise ValueError( | |
| f"env[{i}] is missing required 'name' key. " | |
| f"Each env entry must have at least 'name' and 'value'. Got: {entry}" | |
| ) | |
| for i, entry in enumerate(self.env): | |
| if not isinstance(entry, dict): | |
| raise ValueError( | |
| f"env[{i}] must be a dict, got {type(entry).__name__}: {entry}" | |
| ) | |
| if "name" not in entry: | |
| raise ValueError( | |
| f"env[{i}] is missing required 'name' key. " | |
| f"Each env entry must have at least 'name' and 'value'. Got: {entry}" | |
| ) | |
| if "value" not in entry and "valueFrom" not in entry: | |
| raise ValueError( | |
| f"env[{i}] must have either 'value' or 'valueFrom'. Got: {entry}" | |
| ) |
- Remove redundant get_provider(); callers use .provider property - Replace SparkSession monkey-patch with per-thread session binding - Smart retry: only 5xx/429; fail fast on 401/403 with RBAC hint - Retry ConfigMap + SparkApplication creation (transient K8s errors) - Validate env entries: require name + value or valueFrom (K8s EnvVar) - Independent per-FV job status via CompletedMaterializationJob - Exit 1 on any FV failure so SparkApp CR reflects partial failure - Cleanup logs include kubectl delete command for manual recovery - Lint: pragma allowlist on test fixture URL Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6550 +/- ##
==========================================
- Coverage 46.02% 46.00% -0.03%
==========================================
Files 402 406 +4
Lines 47656 48164 +508
Branches 6741 6814 +73
==========================================
+ Hits 21936 22157 +221
- Misses 24196 24461 +265
- Partials 1524 1546 +22
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…) into merged-spark-e2e
Keep feast-dev#6550 batching; skip FV state transition when _force_local so /materialize-async (already MATERIALIZING) works with SparkApplication. Co-authored-by: Cursor <cursoragent@cursor.com>
| tqdm_builder=lambda length: tqdm(total=length, ncols=100), | ||
| ) | ||
|
|
||
| thread_store.registry.apply_materialization(fv, thread_store.project, start, end) |
There was a problem hiding this comment.
apply_materialization is called twice for each succeeded FV - once by the driver pod, once by the server after the job completes.
There was a problem hiding this comment.
Agreed, the double write was duplicating materialization intervals. The driver pod remains the owner of apply_materialization (needed for per-FV AVAILABLE_ONLINE / partial success). The server batch path now skips the second call when the engine sets applies_materialization=True (SparkApplication only). Other engines still apply on the server as before.
| job = SparkApplicationMaterializationJob( | ||
| job_id, self.config.namespace, self.custom_api | ||
| ) | ||
| self._wait_for_completion(job) |
There was a problem hiding this comment.
This should be async.
This blocks the Feast server thread for the entire duration of the Spark job (up to job_timeout_seconds, default 1 hour). For a server handling multiple clients, this is a scalability concern.
There was a problem hiding this comment.
Agreed this is a real scalability concern for the sync path: _wait_for_completion holds the caller until the SparkApplication finishes.
Making the engine poll itself async would redesign the ComputeEngine.materialize(). The intended non-blocking path is remote materialization in #6590 (POST /materialize-async + registry polling). We'll address true submit-without-wait server behavior there so the async path does not hold a worker thread for the full Spark job.
Would it be okay to leaving this as-is in #6550 and tracked against #6590?
Restore master's per-FV materialize path for engines that do not support batching; keep SparkApplication on the existing batch path via ComputeEngine.supports_batch. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
Take supports_batch isolation; keep _force_local skip for remote async materialize on both batch and per-FV paths. Co-authored-by: Cursor <cursoragent@cursor.com>
… cleanup Replace __end_date__ sentinel with _MaterializationDateRange; fail fast if engine job count mismatches; map UNKNOWN SparkApp state to WAITING; always cleanup ConfigMap/CR after wait. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
Defer kubeconfig load until materialize/cleanup so feast apply can construct the engine without a cluster. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
| # Roll back state to previous value on failure. | ||
| # batch_engine is on PassthroughProvider (concrete); same access as | ||
| # _submit_and_process_materialization_jobs via untyped provider. | ||
| if getattr(provider, "batch_engine").supports_batch: |
There was a problem hiding this comment.
fragile
batch_engine = getattr(provider, "batch_engine", None)
if batch_engine and getattr(batch_engine, "supports_batch", False):
Pod already writes watermarks via applies_materialization; server batch path skips the second call to avoid duplicate intervals. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
Use null-safe getattr so materialize falls back to the per-FV path when batch_engine or supports_batch is absent. Signed-off-by: Aniket Paluskar <apaluska@redhat.com>
Null-safe supports_batch, skip duplicate apply_materialization, lazy K8s client, review hardening.
What this PR does / why we need it:
Core changes
Upstream (feature_store.py):
New engine (spark_application/):
Design decisions
Validated on
Test plan
Which issue(s) this PR fixes:
Checks
git commit -s)Testing Strategy
Misc