fix: Disable materialized odfvs for historical retrieval #5880
Conversation
78ff90f to
c2e42d0
Compare
|
added some explanations for the context |
|
@ntkathole @franciscojavierarceo seen this? Seems like a quota. |
|
@franciscojavierarceo Seems to have passed now 😁 |
| # 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 | ||
| for odfv in all_on_demand_feature_views: | ||
| odfv.write_to_online_store = False |
There was a problem hiding this comment.
i don't love this solution, but i accept it's a pragmatic one.
@copilot can you come up with an alternative?
There was a problem hiding this comment.
adjusted based on copilot suggestion to avoid side effects
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where materialized on-demand feature views (ODFVs) were incorrectly using precomputed values during historical retrieval operations. The fix ensures that ODFVs are always recomputed when querying the offline store for training set generation, aligning with the documented behavior that offline stores do not store precomputed ODFV values.
Changes:
- Force ODFVs to recompute during historical retrieval by disabling
write_to_online_store - Add pytest-xdist as a development dependency
- Add a new Makefile target for running historical retrieval integration tests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/python/feast/feature_store.py | Disables write_to_online_store for all ODFVs before historical retrieval to ensure recomputation |
| pyproject.toml | Adds pytest-xdist development dependency for parallel test execution |
| Makefile | Adds new test target for running historical retrieval integration tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for odfv in all_on_demand_feature_views: | ||
| odfv.write_to_online_store = False |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
good tip, will adjust
Signed-off-by: lukas.valatka <lukas.valatka@cast.ai>
0ff5dc2 to
dafd8d2
Compare
|
@franciscojavierarceo Random ray unit test failure, can we re-run? What's the way of self re-running the pipeline btw, seems akward asking you everytime haha. |
What this PR does / why we need it:
See here. In essence, makes sure ODFV with write_to_online_store=True - meaning, cached - (on-demand feature views) get actually computed when querying offline store (
get_historical_features) for training set generating, since as per docs (and generic expectation), offline stores do not store precomputed on demand feature view historical values (only online stores store their latest values).Which issue(s) this PR fixes:
#5776