Repository navigation
fix: Stop the Ray engine from hiding materialization errors - #7026
Merged
ntkathole merged 3 commits intoOct 18, 2026
Merged
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7026 +/- ##
==========================================
- Coverage 50.99% 50.98% -0.01%
==========================================
Files 455 455
Lines 56979 56990 +11
Branches 8315 8320 +5
==========================================
Hits 29059 29059
- Misses 25935 25946 +11
Partials 1985 1985
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
haoxu0
reviewed
Oct 16, 2026
| last_node = self.build_source_node(self.feature_view) | ||
|
|
||
| # 2. Apply filters (TTL, custom filters) BEFORE aggregation | ||
| # 2. Apply transformations to the raw source data, as materialization does |
Collaborator
There was a problem hiding this comment.
is this order change matching other compute engines?
Contributor
Author
There was a problem hiding this comment.
Yes. Local, Spark and Flink all run the UDF before aggregation (shared FeatureBuilder._build). Only Ray historical retrieval ran it after, and this fixes that.
patelchaitany
force-pushed
the
fix/ray-materialization-errors
branch
from
October 17, 2026 14:27
e3483a6 to
5feefae
Compare
ntkathole
reviewed
Oct 18, 2026
| # Not wrapped in safe_batch_processor: a failed write must fail the job. | ||
| def write_batch_with_serialized_artifacts(batch: pd.DataFrame) -> pd.DataFrame: | ||
| """Write each batch using pre-serialized artifacts.""" | ||
| if batch.empty: |
Member
There was a problem hiding this comment.
for consistency over other code if _is_empty_batch(batch):
ntkathole
reviewed
Oct 18, 2026
|
|
||
| # Combine aggregation results | ||
| if agg_results: | ||
| # Keep the latest timestamps per group; the online write needs them. |
Member
There was a problem hiding this comment.
Tiled aggregation doesn’t preserve created_timestamp_col (fix only touched the standard _fallback_pandas_aggregation).
patelchaitany
force-pushed
the
fix/ray-materialization-errors
branch
2 times, most recently
from
October 18, 2026 13:56
988a73f to
f7e40d2
Compare
ntkathole
approved these changes
Oct 18, 2026
When the online store refused a write, the Ray engine only logged the error and still reported the job as SUCCEEDED. Users kept getting old feature values and had no sign that anything went wrong. Now the error is raised again after it is logged, and the write step is no longer wrapped in safe_batch_processor, which hid every error. A failed write now fails the job, the same as with the Local engine. Fixes feast-dev#7024 Signed-off-by: Chaitany Patel <[email protected]>
The Ray aggregation step kept only the entity keys and the aggregated values. It dropped the event timestamp and created timestamp columns. The online store write needs these columns, so writing an aggregated BatchFeatureView to the online store always failed. The previous commit made this failure visible. Now the aggregation keeps the latest event timestamp and created timestamp for each entity. Signed-off-by: Chaitany Patel <[email protected]>
In materialization, the Ray engine ran the UDF on raw rows before aggregation. In historical retrieval, it ran the UDF after aggregation. So a UDF could only work in one of the two paths. In the other path it raised an error, and safe_batch_processor hid that error, so users got values that were never transformed. Now historical retrieval also runs the UDF on raw rows before aggregation. This matches the compute engine docs and the Local and Spark engines. The transformation step is no longer wrapped in safe_batch_processor, so a UDF error now fails the job. A UDF that used aggregated column names (for example sum_conv_rate) must now use the raw column names (for example conv_rate). Signed-off-by: Chaitany Patel <[email protected]>
ntkathole
force-pushed
the
fix/ray-materialization-errors
branch
from
October 18, 2026 14:31
f7e40d2 to
f446423
Compare
Contributor
|
Thanks @patelchaitany for picking this up so quickly, and for fixing the aggregation and UDF problems it was hiding. And thanks @ntkathole for the review and merge. |
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:
The Ray compute engine hid errors during materialization. A job could report SUCCEEDED even when nothing was written to the online store. Fixing that showed two more bugs that the hidden errors were covering up. This PR fixes all three, one commit each.
1. A failed online write was reported as success (#7024)
When the online store refused a write, Ray logged the error and still reported SUCCEEDED, so users kept getting old values. Now the error is raised again after it is logged, and the write step is no longer wrapped in
safe_batch_processor. A failed write now fails the job, the same as with the Local engine.2. An aggregated BatchFeatureView could never be written to the online store
The Ray aggregation step dropped the event timestamp and created timestamp columns. The online write needs them, so it always failed (bug 1 hid this). Now the aggregation keeps the latest event and created timestamp for each entity.
3. The UDF ran at a different point in each Ray path
Materialization ran the UDF on raw rows before aggregation, but historical retrieval ran it after aggregation. So a UDF could only work in one path. In the other path its error was hidden by
safe_batch_processor, and users got values that were never transformed. Now historical retrieval also runs the UDF before aggregation. This matches the compute engine docs and the Local and Spark engines. The transformation step no longer hides UDF errors.Behavior changes to note
sum_conv_rate) must now use the raw column names (for exampleconv_rate).safe_batch_processoritself is unchanged; other nodes still use it.Which issue(s) this PR fixes:
Fixes #7024
Checks
git commit -s)Testing Strategy
Tests in
sdk/python/tests/component/ray:test_ray_write_node_raises_when_online_write_fails: fails on master, passes with this PR.test_ray_compute_engine_materialize: now uses the aggregated feature names in the schema, and reads the values back from the SQLite online store. Its UDF was removed, because on master it never actually ran (its error was hidden).test_ray_compute_engine_get_historical_features: the UDF now uses the raw columns. Expected values are unchanged. Fails on master, passes with this PR.Ray component tests: 76 passed (non-integration), 14 passed (
--integration).Misc