feat: Add materialization to Transformation On Writes#5436
feat: Add materialization to Transformation On Writes#5436franciscojavierarceo wants to merge 9 commits intofeast-dev:masterfrom
Conversation
Co-Authored-By: Francisco Javier Arceo <farceo@redhat.com>
Co-Authored-By: Francisco Javier Arceo <farceo@redhat.com>
… GenAI documentation Co-Authored-By: Francisco Javier Arceo <farceo@redhat.com>
- Include ODFVs with write_to_online_store=True in _get_feature_views_to_materialize() - Update provider materialize methods to handle ODFVs properly - Add comprehensive unit test to verify ODFV materialize behavior - Fixes GitHub issue feast-dev#5430 where on-write transformations weren't persisting during materialize operations Co-Authored-By: Francisco Javier Arceo <arceofrancisco@gmail.com>
Co-authored-by: Nikhil Kathole <nkathole@redhat.com>
- Update type hints to handle Union[FeatureView, OnDemandFeatureView] - Add proper type checking for ODFV attributes (online, ttl, etc.) - Skip ODFVs in materialize_incremental since they don't have batch sources - Fix registry apply_materialization calls to exclude ODFVs - Remove duplicate imports and fix formatting Co-Authored-By: Francisco Javier Arceo <arceofrancisco@gmail.com>
Co-Authored-By: Francisco Javier Arceo <arceofrancisco@gmail.com>
bb35a71 to
b6314cd
Compare
| ) | ||
| # TODO paging large loads | ||
| for feature_view in feature_views_to_materialize: | ||
| from feast.on_demand_feature_view import OnDemandFeatureView |
There was a problem hiding this comment.
to avoid import twice, can put this at top of the file?
| self, | ||
| feature_views: Optional[List[str]], | ||
| ) -> List[FeatureView]: | ||
| ) -> List[Union[FeatureView, OnDemandFeatureView]]: |
There was a problem hiding this comment.
Interesting that we are returning only FeatureView/OnDemandFeatureView, but in reality when feature_views are not given, we are returning everything including Stream Feature Views.
The return list needs to change here.
| ) | ||
| else: | ||
| for name in feature_views: | ||
| feature_view: Union[FeatureView, OnDemandFeatureView] |
There was a problem hiding this comment.
Could we accept one more parameter to _get_feature_views_to_materialize for returning the list of different types of feature views depending on purpose.
Its really confusing why we are returning different FV types list in if and else blocks .
| start_date, | ||
| end_date, | ||
| ) | ||
| if not isinstance(feature_view, OnDemandFeatureView): |
There was a problem hiding this comment.
Since We are continuing the loop at line 1337&1338, we dont need this condition here.
| if isinstance(feature_view, OnDemandFeatureView): | ||
| continue |
There was a problem hiding this comment.
For my knowledge:
What does it mean that the ODFV cannot be materialized?
| start_date, | ||
| end_date, | ||
| ) | ||
| if not isinstance(feature_view, OnDemandFeatureView): |
There was a problem hiding this comment.
Rather than waiting until this line for ODFV based skip, Continue the looping before getting the provider (line1438).
kind of bringing uniqueness !
| project: str, | ||
| tqdm_builder: Callable[[int], tqdm], | ||
| ) -> None: | ||
| from feast.on_demand_feature_view import OnDemandFeatureView |
There was a problem hiding this comment.
We are reapeating the import.
|
Fixed via #5528 |
What this PR does / why we need it:
Which issue(s) this PR fixes:
#5430
Misc