Skip to content

Commit 046e8e2

Browse files
oavdeevoavdeev
authored
Add nicer validation for repo config (#1392)
* Add nicer validation for repo config Signed-off-by: Oleg Avdeev <oleg.v.avdeev@gmail.com> * add config to docs Signed-off-by: Oleg Avdeev <oleg.v.avdeev@gmail.com> * fix formatting Signed-off-by: Oleg Avdeev <oleg.v.avdeev@gmail.com> * add comments Signed-off-by: Oleg Avdeev <oleg@usrlib.com> * fix config parsing validation & more tests Signed-off-by: Oleg Avdeev <oleg@usrlib.com> * remove test Signed-off-by: Oleg Avdeev <oleg@usrlib.com>
1 parent 4c854b3 commit 046e8e2

File tree

6 files changed

+188
-6
lines changed

6 files changed

+188
-6
lines changed

‎sdk/python/docs/index.rst‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,3 +41,10 @@ Constants
4141
.. automodule:: feast.constants
4242
:members:
4343
:exclude-members: AuthProvider, ConfigMeta
44+
45+
Config
46+
==================
47+
48+
.. automodule:: feast.repo_config
49+
:members:
50+
:exclude-members: load_repo_config

‎sdk/python/feast/infra/provider.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,10 +85,11 @@ def get_provider(config: RepoConfig) -> Provider:
8585
if config.provider == "gcp":
8686
from feast.infra.gcp import Gcp
8787

88-
return Gcp(config.online_store.datastore)
88+
return Gcp(config.online_store.datastore if config.online_store else None)
8989
elif config.provider == "local":
9090
from feast.infra.local_sqlite import LocalSqlite
9191

92+
assert config.online_store is not None
9293
assert config.online_store.local is not None
9394
return LocalSqlite(config.online_store.local)
9495
else:

‎sdk/python/feast/repo_config.py‎

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,29 +3,104 @@
33

44
import yaml
55
from bindr import bind
6+
from jsonschema import ValidationError, validate
67

78

89
class LocalOnlineStoreConfig(NamedTuple):
10+
""" Online store config for local (SQLite-based) online store """
11+
912
path: str
13+
""" str: Path to sqlite db """
1014

1115

1216
class DatastoreOnlineStoreConfig(NamedTuple):
17+
""" Online store config for GCP Datastore """
18+
1319
project_id: str
20+
""" str: GCP Project Id """
1421

1522

1623
class OnlineStoreConfig(NamedTuple):
1724
datastore: Optional[DatastoreOnlineStoreConfig] = None
25+
""" DatastoreOnlineStoreConfig: Optional DatastoreConfig """
1826
local: Optional[LocalOnlineStoreConfig] = None
27+
""" LocalOnlineStoreConfig: Optional local online store config """
1928

2029

2130
class RepoConfig(NamedTuple):
31+
""" Repo config. Typically loaded from `feature_store.yaml` """
32+
2233
metadata_store: str
34+
""" str: Path to metadata store. Can be a local path, or remote object storage path, e.g. gcs://foo/bar """
2335
project: str
36+
""" str: Feast project id. This can be any alphanumeric string up to 16 characters.
37+
You can have multiple independent feature repositories deployed to the same cloud
38+
provider account, as long as they have different project ids.
39+
"""
2440
provider: str
25-
online_store: OnlineStoreConfig
41+
""" str: local or gcp """
42+
online_store: Optional[OnlineStoreConfig] = None
43+
""" OnlineStoreConfig: Online store configuration (optional depending on provider) """
44+
45+
46+
# This is the JSON Schema for config validation. We use this to have nice detailed error messages
47+
# for config validation, something that bindr unfortunately doesn't provide out of the box.
48+
#
49+
# The schema should match the namedtuple structure above. It could technically even be inferred from
50+
# the types above automatically; but for now we choose a more tedious but less magic path of
51+
# providing the schema manually.
52+
53+
config_schema = {
54+
"type": "object",
55+
"properties": {
56+
"project": {"type": "string"},
57+
"metadata_store": {"type": "string"},
58+
"provider": {"type": "string"},
59+
"online_store": {
60+
"type": "object",
61+
"properties": {
62+
"local": {
63+
"type": "object",
64+
"properties": {"path": {"type": "string"}},
65+
"additionalProperties": False,
66+
},
67+
"datastore": {
68+
"type": "object",
69+
"properties": {"project_id": {"type": "string"}},
70+
"additionalProperties": False,
71+
},
72+
},
73+
"additionalProperties": False,
74+
},
75+
},
76+
"required": ["project"],
77+
"additionalProperties": False,
78+
}
79+
80+
81+
class FeastConfigError(Exception):
82+
def __init__(self, error_message, error_path, config_path):
83+
self._error_message = error_message
84+
self._error_path = error_path
85+
self._config_path = config_path
86+
super().__init__(self._error_message)
87+
88+
def __str__(self) -> str:
89+
if self._error_path:
90+
return f'{self._error_message} under {"->".join(self._error_path)} in {self._config_path}'
91+
else:
92+
return f"{self._error_message} in {self._config_path}"
93+
94+
def __repr__(self) -> str:
95+
return f"FeastConfigError({repr(self._error_message)}, {repr(self._error_path)}, {repr(self._config_path)})"
2696

2797

2898
def load_repo_config(repo_path: Path) -> RepoConfig:
29-
with open(repo_path / "feature_store.yaml") as f:
99+
config_path = repo_path / "feature_store.yaml"
100+
with open(config_path) as f:
30101
raw_config = yaml.safe_load(f)
31-
return bind(RepoConfig, raw_config)
102+
try:
103+
validate(raw_config, config_schema)
104+
return bind(RepoConfig, raw_config)
105+
except ValidationError as e:
106+
raise FeastConfigError(e.message, e.absolute_path, config_path)

‎sdk/python/setup.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
"google",
4848
"bindr",
4949
"mmh3",
50+
"jsonschema",
5051
]
5152

5253
# README file from Feast repo root directory

‎sdk/python/tests/cli/test_datastore.py‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,6 @@ def test_basic(self) -> None:
4646
project: {self._project_id}
4747
metadata_store: {data_path / "metadata.db"}
4848
provider: gcp
49-
online_store:
50-
datastore:
5149
"""
5250
)
5351
)

‎sdk/python/tests/test_repo_config.py‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
import re
2+
import tempfile
3+
from pathlib import Path
4+
from textwrap import dedent
5+
from typing import Optional
6+
7+
from feast.repo_config import FeastConfigError, load_repo_config
8+
9+
10+
class TestRepoConfig:
11+
def _test_config(self, config_text, expect_error: Optional[str]):
12+
"""
13+
Try loading a repo config and check raised error against a regex.
14+
"""
15+
with tempfile.TemporaryDirectory() as repo_dir_name:
16+
17+
repo_path = Path(repo_dir_name)
18+
19+
repo_config = repo_path / "feature_store.yaml"
20+
21+
repo_config.write_text(config_text)
22+
error = None
23+
try:
24+
load_repo_config(repo_path)
25+
except FeastConfigError as e:
26+
error = e
27+
28+
if expect_error is not None:
29+
assert re.search(expect_error, str(error)) is not None
30+
else:
31+
assert error is None
32+
33+
def test_basic(self) -> None:
34+
self._test_config(
35+
dedent(
36+
"""
37+
project: foo
38+
metadata_store: "metadata.db"
39+
provider: local
40+
online_store:
41+
local:
42+
path: "online_store.db"
43+
"""
44+
),
45+
expect_error=None,
46+
)
47+
48+
self._test_config(
49+
dedent(
50+
"""
51+
project: foo
52+
metadata_store: "metadata.db"
53+
provider: gcp
54+
"""
55+
),
56+
expect_error=None,
57+
)
58+
59+
def test_errors(self) -> None:
60+
self._test_config(
61+
dedent(
62+
"""
63+
project: foo
64+
metadata_store: "metadata.db"
65+
provider: local
66+
online_store:
67+
local:
68+
that_field_should_not_be_here: yes
69+
path: "online_store.db"
70+
"""
71+
),
72+
expect_error=r"'that_field_should_not_be_here' was unexpected.*online_store->local",
73+
)
74+
75+
self._test_config(
76+
dedent(
77+
"""
78+
project: foo
79+
metadata_store: "metadata.db"
80+
provider: local
81+
online_store:
82+
local:
83+
path: 100500
84+
"""
85+
),
86+
expect_error=r"100500 is not of type 'string'",
87+
)
88+
89+
self._test_config(
90+
dedent(
91+
"""
92+
metadata_store: "metadata.db"
93+
provider: local
94+
online_store:
95+
local:
96+
path: foo
97+
"""
98+
),
99+
expect_error=r"'project' is a required property",
100+
)

0 commit comments

Comments
 (0)