Skip to content

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

Conversation

franciscojavierarceo
Copy link
Member

What this PR does / why we need it:

Which issue(s) this PR fixes:

#5430

Misc

@franciscojavierarceo franciscojavierarceo requested a review from a team as a code owner June 9, 2025 15:45
@devin-ai-integration devin-ai-integration bot requested a review from a team as a code owner June 9, 2025 16:26
devin-ai-integration bot and others added 9 commits June 9, 2025 17:52
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>
@devin-ai-integration devin-ai-integration bot force-pushed the devin/1749466479-fix-odfv-on-write-transformations branch from bb35a71 to b6314cd Compare June 9, 2025 18:01
@@ -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
Copy link
Collaborator

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]]:
Copy link
Contributor

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]
Copy link
Contributor

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):
Copy link
Contributor

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.

Comment on lines +1337 to +1338
if isinstance(feature_view, OnDemandFeatureView):
continue
Copy link
Contributor

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):
Copy link
Contributor

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
Copy link
Contributor

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.

@ntkathole
Copy link
Member

Fixed via #5528

@ntkathole ntkathole closed this Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants