-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Add materialization to Transformation On Writes #5436
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add materialization to Transformation On Writes #5436
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
@@ -1312,6 +1332,11 @@ def materialize_incremental( | |||
) | |||
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid import twice, can put this at top of the file?
@@ -656,7 +656,7 @@ def _make_inferences( | |||
def _get_feature_views_to_materialize( | |||
self, | |||
feature_views: Optional[List[str]], | |||
) -> List[FeatureView]: | |||
) -> List[Union[FeatureView, OnDemandFeatureView]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than waiting until this line for ODFV based skip, Continue
the looping before getting the provider (line1438).
kind of bringing uniqueness !
start_date: datetime, | ||
end_date: datetime, | ||
registry: BaseRegistry, | ||
project: str, | ||
tqdm_builder: Callable[[int], tqdm], | ||
) -> None: | ||
from feast.on_demand_feature_view import OnDemandFeatureView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are reapeating the import.
Fixed via #5528 |
What this PR does / why we need it:
Which issue(s) this PR fixes:
#5430
Misc