Skip to content

fix: Disable materialized odfvs for historical retrieval #5880

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

Open
astronautas wants to merge 1 commit into
base: master
Choose a base branch
from
+236 −570
Open

fix: Disable materialized odfvs for historical retrieval #5880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,17 @@ test-python-universal-spark: ## Run Python Spark integration tests
not test_snowflake" \
sdk/python/tests

test-python-historical-retrieval:
## Run Python historical retrieval integration tests
PYTHONPATH='.' \
FULL_REPO_CONFIGS_MODULE=sdk.python.feast.infra.offline_stores.contrib.spark_repo_configuration \
PYTEST_PLUGINS=feast.infra.offline_stores.contrib.spark_offline_store.tests \
python -m pytest -n 8 --integration \
-k "test_historical_retrieval_with_validation or \
test_historical_features_persisting or \
test_historical_retrieval_fails_on_validation" \
sdk/python/tests

test-python-universal-trino: ## Run Python Trino integration tests
PYTHONPATH='.' \
FULL_REPO_CONFIGS_MODULE=sdk.python.feast.infra.offline_stores.contrib.trino_repo_configuration \
Expand Down
5 changes: 5 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,3 +264,8 @@ exclude = [
".pyi",
"protos",
"sdk/python/feast/embedded_go/lib"]

[dependency-groups]
dev = [
"pytest-xdist>=3.8.0",
]
12 changes: 12 additions & 0 deletions sdk/python/feast/feature_store.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
import asyncio
import copy
import itertools
import os
import warnings
Expand Down Expand Up @@ -1233,6 +1234,17 @@ def get_historical_features(
# TODO(achal): _group_feature_refs returns the on demand feature views, but it's not passed into the provider.
# This is a weird interface quirk - we should revisit the `get_historical_features` to
# pass in the on demand feature views as well.

# Deliberately disable writing to online store for ODFVs during historical retrieval
# since it's not applicable in this context.
# This does not change the output, since it forces to recompute ODFVs on historical retrieval
# but that is fine, since ODFVs precompute does not to work for historical retrieval (as per docs), only for online retrieval
# Copy to avoid side effects outside of this method
all_on_demand_feature_views = copy.deepcopy(all_on_demand_feature_views)

for odfv in all_on_demand_feature_views:
odfv.write_to_online_store = False
Copy link
Member

@franciscojavierarceo franciscojavierarceo Jan 20, 2026

Choose a reason for hiding this comment

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

i don't love this solution, but i accept it's a pragmatic one.

@copilot can you come up with an alternative?

Copy link
Contributor Author

@astronautas astronautas Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

adjusted based on copilot suggestion to avoid side effects

Comment on lines +1245 to +1246
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

Mutating the write_to_online_store property of ODFVs in-place may have unintended side effects if these objects are reused elsewhere. Consider creating copies of the ODFVs with the modified property or restoring the original values after use to avoid potential issues with shared state.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@astronautas astronautas Jan 21, 2026

Choose a reason for hiding this comment

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

good tip, will adjust

Copy link
Contributor Author

@astronautas astronautas Jan 21, 2026

Choose a reason for hiding this comment

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

adjusted


fvs, odfvs = utils._group_feature_refs(
_feature_refs,
all_feature_views,
Expand Down
Loading
Loading