fix: keep HybridOnlineStore routing_tag across FeatureViews - #6769
Open
jang-hs wants to merge 1 commit into
Open
fix: keep HybridOnlineStore routing_tag across FeatureViews#6769jang-hs wants to merge 1 commit into
jang-hs wants to merge 1 commit into
Conversation
HybridOnlineStore.update() applied the first FeatureView and then failed on the second with ValueError: FeatureView must have a 'tribe' tag to use HybridOnlineStore no matter how the views were tagged. The message reports 'tribe' — the fallback, not the configured routing_tag — because config.online_store is no longer the HybridOnlineStoreConfig by the time the second view is read. Two causes: 1. _prepare_repo_conf mutated the caller's RepoConfig. `rconfig = config` is an alias, so both the attribute assignment and the __dict__ writes landed on the caller's object and replaced online_store with the selected backend's config. It also injected `type` into online_store.conf, which belongs to the user's config. 2. update() and teardown() rebound the `config` parameter inside their loops, so the next iteration read routing_tag off the narrowed config even once the mutation was gone. Build the returned mapping from a copy, construct a new dict for the backend conf, and assign the per-backend RepoConfig to a local name in the loops. Behaviour is otherwise unchanged: the returned kwargs are identical to what the mutating version produced. Adds a unit regression test covering both the multi-FeatureView routing and the caller's config staying intact. Both fail on master. Signed-off-by: Jade <retrorca@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6768
What this fixes
HybridOnlineStore.update()applies the firstFeatureViewand then fails on the second, no matter how the views are tagged:The message reports
'tribe'even when the repo config sets a customrouting_tag, because that string is the fallback inGetting the fallback means
config.online_storeis no longer theHybridOnlineStoreConfigby the time the second view is read. In practice this makes the store unusable for any repo with more than oneFeatureView.Root cause
1.
_prepare_repo_confmutated the caller'sRepoConfig.rconfig = configis an alias, not a copy, sorconfig.online_config = ...and thedata = rconfig.__dict__writes all landed on the caller's object — replacingonline_storewith the selected backend's config. It also injectedtypeintoonline_store.conf, which belongs to the user's config and is read again on the next lookup.2.
update()andteardown()rebound theconfigparameter inside their loops. Even with (1) fixed, the next iteration would receive the narrowed config instead of the hybrid one.online_write_batch()andonline_read()rebind as well, but those are single-use per call; fixing (1) is what makes them safe, so they are left as they are to keep the diff small.The change
_prepare_repo_confbuilds its result fromdict(config.__dict__)and constructs a new dict for the backend conf instead of injecting into the caller's. The returned kwargs are identical to what the mutating version produced —online_configandonline_storeare both set to the selected backend's conf, exactly as before — so nothing downstream changes.update()andteardown()assign the per-backendRepoConfigto a localstore_configrather than rebindingconfig.Tests
New unit test at
sdk/python/tests/unit/infra/online_store/test_hybrid_online_store.py:test_update_routes_every_feature_view— two views with a customrouting_tag, one per backend; asserts both backends'update()are called once and the caller'srouting_tagsurvives.test_prepare_repo_conf_does_not_mutate_caller_config— asserts the caller'sonline_storeobject and itsconfdict are untouched.Both fail on
masterand pass with this change:ruff check,ruff format --check, andmypyare clean on both files (the mypy errors that remain in the tree are pre-existing and in other modules).The existing
sdk/python/tests/integration/online_store/test_hybrid_online_store.pycovers the write/read path; I could not run it locally as it needstestcontainers/Docker, but this change does not alter the config those paths receive.