Skip to content

feat: Pass requested On-Demand Feature Views to Provider get_historical_features#5803

Open
Shizoqua wants to merge 5 commits intofeast-dev:masterfrom
Shizoqua:feat/pass-odfvs-to-provider
Open

feat: Pass requested On-Demand Feature Views to Provider get_historical_features#5803
Shizoqua wants to merge 5 commits intofeast-dev:masterfrom
Shizoqua:feat/pass-odfvs-to-provider

Conversation

@Shizoqua
Copy link

@Shizoqua Shizoqua commented Dec 31, 2025 •
edited by devin-ai-integration bot
Loading

Summary

This PR fixes an interface/plumbing gap in historical retrieval by explicitly passing the requested On-Demand Feature Views (ODFVs) from FeatureStore.get_historical_features() into the provider’s get_historical_features() implementation.

What changed

  • Updated Provider.get_historical_features(...) signature to include:
    • on_demand_feature_views: List[OnDemandFeatureView]
  • Updated FeatureStore.get_historical_features() to pass the computed on_demand_feature_views list into the provider call.
  • Updated PassthroughProvider.get_historical_features() to accept the new argument (ignored for passthrough delegation).
  • Updated the test FooProvider implementation to match the new provider interface.
  • Added a unit test that asserts the provider receives the expected ODFVs list:
    • sdk/python/tests/unit/test_feature_store_passes_odfvs_to_provider.py

Why

Providers may need the explicit set of requested ODFVs to correctly handle or route historical retrieval logic. Previously, only the combined feature_views list was passed, which made it awkward/impossible for providers to distinguish which ODFVs were requested.

Testing

  • Added unit test test_feature_store_passes_on_demand_feature_views_to_provider.
Open with Devin

@Shizoqua Shizoqua requested a review from a team as a code owner December 31, 2025 00:15
@Shizoqua Shizoqua force-pushed the feat/pass-odfvs-to-provider branch from e63b40b to b2d82b6 Compare December 31, 2025 00:18
@Shizoqua Shizoqua changed the title Feat: Pass requested On-Demand Feature Views to Provider get_historical_features Feat: Pass requested On-Demand Feature Views Dec 31, 2025
@Shizoqua Shizoqua changed the title Feat: Pass requested On-Demand Feature Views Feat: Pass requested On-Demand Feature Views from FeatureStore.get_historical_features() Dec 31, 2025
@Shizoqua Shizoqua changed the title Feat: Pass requested On-Demand Feature Views from FeatureStore.get_historical_features() Feat: Pass requested On-Demand Feature Views to Provider get_historical_features Dec 31, 2025
@Shizoqua Shizoqua changed the title Feat: Pass requested On-Demand Feature Views to Provider get_historical_features feat: Pass requested On-Demand Feature Views to Provider get_historical_features Jan 1, 2026
@Shizoqua
Copy link
Author

Shizoqua commented Jan 1, 2026

Good day @franciscojavierarceo
I would appreciate it if my PR is looked at and reviewed if there is any comment you'd want to give in relations to the PR would be highly appreciated.

@Shizoqua Shizoqua force-pushed the feat/pass-odfvs-to-provider branch from 0458b97 to 1501fd4 Compare January 12, 2026 19:34
@Shizoqua
Copy link
Author

Shizoqua commented Jan 12, 2026

Good day @franciscojavierarceo
I would appreciate it if my PR is looked at and reviewed.

1 similar comment
@Shizoqua
Copy link
Author

Shizoqua commented Jan 16, 2026

Good day @franciscojavierarceo
I would appreciate it if my PR is looked at and reviewed.

@Shizoqua
Copy link
Author

Shizoqua commented Jan 19, 2026

Hello,
Good day @franciscojavierarceo
Just a quick reminder of my PR getting reviewed.

@HaoXuAI
Copy link
Collaborator

HaoXuAI commented Jan 19, 2026

Can you explain why you need this argument?

@Shizoqua
Copy link
Author

Shizoqua commented Jan 20, 2026

Can you explain why you need this argument?

The new on_demand_feature_views argument is needed because FeatureStore.get_historical_features() already resolves which OnDemandFeatureViews are actually requested (via _group_feature_refs), but historically we were not passing that information to the Provider.
Without this argument, the Provider only sees feature_views + feature_refs and has to:

  • either re-resolve ODFVs from the feature refs (i.e. duplicating logic), or
  • conservatively treat ODFVs as part of the full graph and potentially execute transformations for ODFVs that aren’t requested, which can add unnecessary joins/transforms and overhead.

Passing on_demand_feature_views makes the API explicit so provider implementations that can optimize or need this for correct planning like compute-engine based retrieval can do so without re-deriving it.

@HaoXuAI
Copy link
Collaborator

HaoXuAI commented Jan 21, 2026

Can you explain why you need this argument?

The new on_demand_feature_views argument is needed because FeatureStore.get_historical_features() already resolves which OnDemandFeatureViews are actually requested (via _group_feature_refs), but historically we were not passing that information to the Provider. Without this argument, the Provider only sees feature_views + feature_refs and has to:

  • either re-resolve ODFVs from the feature refs (i.e. duplicating logic), or
  • conservatively treat ODFVs as part of the full graph and potentially execute transformations for ODFVs that aren’t requested, which can add unnecessary joins/transforms and overhead.

Passing on_demand_feature_views makes the API explicit so provider implementations that can optimize or need this for correct planning like compute-engine based retrieval can do so without re-deriving it.

Although I don’t fully support this ad hoc change, since it’s not user-facing, I’m okay with approving it. Could you please add a TODO and create a follow-up issue to flag it for a proper long-term fix?

@Shizoqua
Copy link
Author

Shizoqua commented Jan 22, 2026

Although I don’t fully support this ad hoc change, since it’s not user-facing, I’m okay with approving it. Could you please add a TODO and create a follow-up issue to flag it for a proper long-term fix?

Okay, I'm on it now

@Shizoqua Shizoqua mentioned this pull request Jan 22, 2026
Open
@Shizoqua
Copy link
Author

Shizoqua commented Jan 22, 2026 •
edited
Loading

Although I don’t fully support this ad hoc change, since it’s not user-facing, I’m okay with approving it. Could you please add a TODO and create a follow-up issue to flag it for a proper long-term fix?

Added a TODO in FeatureStore.get_historical_features to flag this as an interim interface change. I’ve also open a follow-up issue to track a proper long-term cleanup of the historical retrieval Provider API (request object / clearer contract) and will link it here once created.

@Shizoqua Shizoqua force-pushed the feat/pass-odfvs-to-provider branch from 0bc17e6 to 970cd02 Compare January 30, 2026 11:53
devin-ai-integration[bot]
devin-ai-integration bot reviewed Jan 30, 2026
View reviewed changes
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional flags.

Open in Devin Review

@Shizoqua
Copy link
Author

Shizoqua commented Jan 31, 2026 •
edited
Loading

Hello,
Good day @franciscojavierarceo
Just a quick reminder of my PR getting reviewed or processed

@andrevus andrevus mentioned this pull request Mar 12, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments

@franciscojavierarceo franciscojavierarceo franciscojavierarceo approved these changes

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Shizoqua @HaoXuAI @franciscojavierarceo