Skip to content

Commit dd428a5

Browse files
jang-hsjang-hs
committed
fix: keep HybridOnlineStore routing_tag across FeatureViews
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>
1 parent 8ab92e8 commit dd428a5

2 files changed

Lines changed: 123 additions & 9 deletions

File tree

‎sdk/python/feast/infra/online_stores/hybrid_online_store/hybrid_online_store.py‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -139,21 +139,28 @@ def _prepare_repo_conf(self, config: RepoConfig, online_store_type: str):
139139
"""
140140
Prepare a RepoConfig for the selected online store backend.
141141
142+
The caller's RepoConfig is left untouched: the returned mapping is built
143+
from a copy. Mutating it in place replaced the caller's HybridOnlineStore
144+
config with the selected backend's config, so every later lookup of
145+
``routing_tag`` fell back to the "tribe" default.
146+
142147
Args:
143148
config: The original Feast RepoConfig.
144149
online_store_type: The type of the online store backend to use.
145150
Returns:
146151
A dictionary representing the updated RepoConfig for the selected backend.
147152
"""
148-
rconfig = config
153+
online_config = config.online_config
149154
for online_store in config.online_store.online_stores:
150155
if online_store.type.split(".")[-1].lower() == online_store_type.lower():
151-
rconfig.online_config = online_store.conf
152-
rconfig.online_config["type"] = online_store.type
153-
data = rconfig.__dict__
156+
# New dict rather than a mutated one: ``conf`` belongs to the
157+
# caller's config and is read again on the next lookup.
158+
online_config = {**online_store.conf, "type": online_store.type}
159+
data = dict(config.__dict__)
154160
data["registry"] = data["registry_config"]
155161
data["offline_store"] = data["offline_config"]
156-
data["online_store"] = data["online_config"]
162+
data["online_config"] = online_config
163+
data["online_store"] = online_config
157164
return data
158165

159166
def _get_routing_tag_value(self, table: FeatureView, config: RepoConfig):
@@ -275,9 +282,12 @@ def update(
275282
)
276283
online_store = self._get_online_store(tribe, config)
277284
if online_store:
278-
config = RepoConfig(**self._prepare_repo_conf(config, tribe))
285+
# Local name: rebinding `config` here would feed the next
286+
# iteration the selected backend's config instead of the hybrid
287+
# one, losing `routing_tag` from the second FeatureView on.
288+
store_config = RepoConfig(**self._prepare_repo_conf(config, tribe))
279289
online_store.update(
280-
config,
290+
store_config,
281291
tables_to_delete,
282292
tables_to_keep,
283293
entities_to_delete,
@@ -323,5 +333,7 @@ def teardown(
323333
if tribe.lower() == store_type.split(".")[-1].lower():
324334
online_store = self._get_online_store(tribe, config)
325335
if online_store:
326-
config = RepoConfig(**self._prepare_repo_conf(config, tribe))
327-
online_store.teardown(config, tables, entities)
336+
store_config = RepoConfig(
337+
**self._prepare_repo_conf(config, tribe)
338+
)
339+
online_store.teardown(store_config, tables, entities)

‎sdk/python/tests/unit/infra/online_store/test_hybrid_online_store.py‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
from unittest.mock import patch
2+
3+
import pytest
4+
5+
from feast import Entity, FeatureView, Field, FileSource, RepoConfig, ValueType
6+
from feast.infra.online_stores.hybrid_online_store.hybrid_online_store import (
7+
HybridOnlineStore,
8+
HybridOnlineStoreConfig,
9+
)
10+
from feast.types import PrimitiveFeastType
11+
12+
ROUTING_TAG = "backend"
13+
14+
15+
@pytest.fixture
16+
def entity():
17+
return Entity(name="id", join_keys=["id"], value_type=ValueType.INT64)
18+
19+
20+
def _feature_view(name: str, backend: str, entity: Entity) -> FeatureView:
21+
return FeatureView(
22+
name=name,
23+
entities=[entity],
24+
schema=[Field(name="feature1", dtype=PrimitiveFeastType.INT64)],
25+
online=True,
26+
tags={ROUTING_TAG: backend},
27+
source=FileSource(
28+
path="/tmp/feast_hybrid_test.parquet",
29+
event_timestamp_column="event_timestamp",
30+
),
31+
)
32+
33+
34+
@pytest.fixture
35+
def repo_config():
36+
return RepoConfig(
37+
registry="test-registry.db",
38+
project="test_project",
39+
provider="local",
40+
online_store=HybridOnlineStoreConfig(
41+
routing_tag=ROUTING_TAG,
42+
online_stores=[
43+
HybridOnlineStoreConfig.OnlineStoresWithConfig(
44+
type="redis",
45+
conf={"redis_type": "redis", "connection_string": "localhost:6379"},
46+
),
47+
HybridOnlineStoreConfig.OnlineStoresWithConfig(
48+
type="sqlite",
49+
conf={"path": "/tmp/feast_hybrid_test.db"},
50+
),
51+
],
52+
),
53+
offline_store=None,
54+
)
55+
56+
57+
def test_prepare_repo_conf_does_not_mutate_caller_config(repo_config):
58+
"""The selected backend's config must not leak back into the caller's config."""
59+
original_online_store = repo_config.online_store
60+
original_redis_conf = dict(repo_config.online_store.online_stores[0].conf)
61+
62+
HybridOnlineStore()._prepare_repo_conf(repo_config, "redis")
63+
64+
assert repo_config.online_store is original_online_store
65+
assert repo_config.online_store.routing_tag == ROUTING_TAG
66+
# `type` used to be injected into the caller's own conf dict.
67+
assert repo_config.online_store.online_stores[0].conf == original_redis_conf
68+
69+
70+
def test_update_routes_every_feature_view(repo_config, entity):
71+
"""Regression: routing used to break from the second FeatureView onwards.
72+
73+
`update()` rebound `config` to the selected backend's RepoConfig, so the next
74+
iteration read `routing_tag` off a config that no longer had one. It fell back
75+
to the "tribe" default, found no such tag, and raised
76+
"FeatureView must have a 'tribe' tag to use HybridOnlineStore".
77+
"""
78+
tables = [
79+
_feature_view("fv_redis", "redis", entity),
80+
_feature_view("fv_sqlite", "sqlite", entity),
81+
]
82+
83+
with (
84+
patch(
85+
"feast.infra.online_stores.redis.RedisOnlineStore.update"
86+
) as redis_update,
87+
patch(
88+
"feast.infra.online_stores.sqlite.SqliteOnlineStore.update"
89+
) as sqlite_update,
90+
):
91+
HybridOnlineStore().update(
92+
config=repo_config,
93+
tables_to_delete=[],
94+
tables_to_keep=tables,
95+
entities_to_delete=[],
96+
entities_to_keep=[entity],
97+
partial=False,
98+
)
99+
100+
assert redis_update.call_count == 1
101+
assert sqlite_update.call_count == 1
102+
assert repo_config.online_store.routing_tag == ROUTING_TAG

0 commit comments

Comments
 (0)