FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat: Pass requested On-Demand Feature Views to Provider `get_historical_features` by Shizoqua · Pull Request #5803 · feast-dev/feast · GitHub

  1. feast-dev
  2. feast

Repository navigation

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

Open
Shizoqua wants to merge 6 commits into
feast-dev:masterfrom
Shizoqua:feat/pass-odfvs-to-provider
ConversationCommits (6)ChecksFiles changed
Open

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

Conversation

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

Copy link
Copy Markdown

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 requested a review from a team as a code owner December 31, 2025 00:15
Shizoqua force-pushed the feat/pass-odfvs-to-provider branch from e63b40b to b2d82b6 Compare December 31, 2025 00:18
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 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 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 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 commented Jan 1, 2026

Copy link
Copy Markdown
Author

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 force-pushed the feat/pass-odfvs-to-provider branch from 0458b97 to 1501fd4 Compare January 12, 2026 19:34

Shizoqua commented Jan 12, 2026

Copy link
Copy Markdown
Author

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

1 similar comment

Shizoqua commented Jan 16, 2026

Copy link
Copy Markdown
Author

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

Shizoqua commented Jan 19, 2026

Copy link
Copy Markdown
Author

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

haoxu0 commented Jan 19, 2026

Copy link
Copy Markdown
Collaborator

Can you explain why you need this argument?

Shizoqua commented Jan 20, 2026

Copy link
Copy Markdown
Author

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.

haoxu0 commented Jan 21, 2026

Copy link
Copy Markdown
Collaborator

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 commented Jan 22, 2026

Copy link
Copy Markdown
Author

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 mentioned this pull request Jan 22, 2026
Open

Shizoqua commented Jan 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

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.

feat: pass ODFVs to provider for historical retrieval
4f03d48 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
feat: pass requested ODFVs into provider.get_historical_features
7dcf9bf 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
chore: fix ruff lint in ODFV provider tests
f352fd6 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
style: format ODFV provider test
05024d2 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
docs: add TODO for long-term historical retrieval API cleanup
970cd02 
Signed-off-by: Shizoqua <hr.lanreshittu@yahoo.com>
Shizoqua force-pushed the feat/pass-odfvs-to-provider branch from 0bc17e6 to 970cd02 Compare January 30, 2026 11:53
devin-ai-integration Bot reviewed Jan 30, 2026
View reviewed changes

devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

✅ 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.

Shizoqua commented Jan 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

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

andrevus mentioned this pull request Mar 12, 2026
Closed

Shizoqua commented Mar 18, 2026

Copy link
Copy Markdown
Author

Hello,
Good day
Please can my PR get reviewed?

franciscojavierarceo requested changes Mar 25, 2026
View reviewed changes

franciscojavierarceo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

tests are failing

This branch has not been deployed

No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

devin-ai-integration[bot] devin-ai-integration[bot] left review comments
franciscojavierarceo franciscojavierarceo requested 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

Back | FazBrowse Home | New Git URL