Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Open
Shizoqua wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Shizoqua wants to merge 5 commits into from
+71 −0

Conversation

@Shizoqua
Copy link

@Shizoqua Shizoqua commented Dec 31, 2025

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.

@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
feat: pass ODFVs to provider for historical retrieval
1a93f6b 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
@Shizoqua
feat: pass requested ODFVs into provider.get_historical_features
01a8320 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
@Shizoqua
chore: fix ruff lint in ODFV provider tests
c52cee3 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
@Shizoqua
style: format ODFV provider test
1501fd4 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@franciscojavierarceo franciscojavierarceo Awaiting requested review from franciscojavierarceo

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