Skip to content
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.

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

Add nicer validation for repo config #1392

Merged
merged 6 commits into from
Mar 20, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
7 changes: 7 additions & 0 deletions sdk/python/docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,10 @@ Constants
.. automodule:: feast.constants
:members:
:exclude-members: AuthProvider, ConfigMeta

Config
==================

.. automodule:: feast.repo_config
:members:
:exclude-members: load_repo_config
73 changes: 71 additions & 2 deletions sdk/python/feast/repo_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,98 @@

import yaml
from bindr import bind
from jsonschema import ValidationError, validate


class LocalOnlineStoreConfig(NamedTuple):
""" Online store config for local (SQLite-based) online store """
path: str
""" str: Path to sqlite db """


class DatastoreOnlineStoreConfig(NamedTuple):
""" Online store config for GCP Datastore """
project_id: str
""" str: GCP Project Id """


class OnlineStoreConfig(NamedTuple):
datastore: Optional[DatastoreOnlineStoreConfig] = None
""" DatastoreOnlineStoreConfig: Optional DatastoreConfig """
local: Optional[LocalOnlineStoreConfig] = None
""" LocalOnlineStoreConfig: Optional local online store config """


class RepoConfig(NamedTuple):
""" Repo config. Typically loaded from `feature_store.yaml` """
metadata_store: str
""" str: Path to metadata store. Can be a local path, or remote object storage path """
woop marked this conversation as resolved.
Show resolved Hide resolved
project: str
""" str: Feast project id. This can be any alphanumeric string up to 16 characters. """
woop marked this conversation as resolved.
Show resolved Hide resolved
provider: str
""" str: local or gcp """
online_store: OnlineStoreConfig
""" OnlineStoreConfig: Online store configuration (optional depending on provider) """


# This is the JSON Schema for config validation. We use this to have nice detailed error messages
# for config validation, something that bindr unfortunately doesn't provide out of the box.
#
# The schema should match the namedtuple structure above. It could technically even be inferred from
# the types above automatically; but for now we choose a more tedious but less magic path of
# providing the schema manually.

config_schema = {
"type": "object",
"properties": {
"project": {"type": "string"},
woop marked this conversation as resolved.
Show resolved Hide resolved
"metadata_store": {"type": "string"},
"provider": {"type": "string"},
"online_store": {
"type": "object",
"properties": {
"local": {
"type": "object",
"properties": {"path": {"type": "string"}},
"additionalProperties": False,
},
"datastore": {
"type": "object",
"properties": {"project_id": {"type": "string"}},
"additionalProperties": False,
},
},
"additionalProperties": False,
},
},
"required": ["project"],
"additionalProperties": False,
}


class FeastConfigError(Exception):
def __init__(self, error_message, error_path, config_path):
self._error_message = error_message
self._error_path = error_path
self._config_path = config_path
super().__init__(self._error_message)

def __str__(self) -> str:
if self._error_path:
return f'{self._error_message} under {"->".join(self._error_path)} in {self._config_path}'
else:
return f"{self._error_message} in {self._config_path}"

def __repr__(self) -> str:
return f"FeastConfigError({repr(self._error_message)}, {repr(self._error_path)}, {repr(self._config_path)})"


def load_repo_config(repo_path: Path) -> RepoConfig:
with open(repo_path / "feature_store.yaml") as f:
config_path = repo_path / "feature_store.yaml"
with open(config_path) as f:
raw_config = yaml.safe_load(f)
return bind(RepoConfig, raw_config)
try:
validate(raw_config, config_schema)
return bind(RepoConfig, raw_config)
except ValidationError as e:
raise FeastConfigError(e.message, e.absolute_path, config_path)
1 change: 1 addition & 0 deletions sdk/python/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"kubernetes==12.0.*",
"bindr",
"mmh3",
"jsonschema",
]

# README file from Feast repo root directory
Expand Down
88 changes: 88 additions & 0 deletions sdk/python/tests/test_repo_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import re
import tempfile
from pathlib import Path
from textwrap import dedent
from typing import Optional

from feast.repo_config import FeastConfigError, load_repo_config


class TestRepoConfig:
def _test_config(self, config_text, expect_error: Optional[str]):
"""
Try loading a repo config and check raised error against a regex.
"""
with tempfile.TemporaryDirectory() as repo_dir_name:

repo_path = Path(repo_dir_name)

repo_config = repo_path / "feature_store.yaml"

repo_config.write_text(config_text)
error = None
try:
load_repo_config(repo_path)
except FeastConfigError as e:
error = e

if expect_error is not None:
assert re.search(expect_error, str(error)) is not None
else:
assert error is None

def test_basic(self) -> None:
self._test_config(
dedent(
"""
project: foo
metadata_store: "metadata.db"
provider: local
online_store:
local:
path: "online_store.db"
"""
),
expect_error=None,
)

self._test_config(
dedent(
"""
project: foo
metadata_store: "metadata.db"
provider: local
online_store:
local:
that_field_should_not_be_here: yes
path: "online_store.db"
"""
),
expect_error=r"'that_field_should_not_be_here' was unexpected.*online_store->local",
)

self._test_config(
dedent(
"""
project: foo
metadata_store: "metadata.db"
provider: local
online_store:
local:
path: 100500
"""
),
expect_error=r"100500 is not of type 'string'",
)

self._test_config(
dedent(
"""
metadata_store: "metadata.db"
provider: local
online_store:
local:
path: foo
"""
),
expect_error=r"'project' is a required property",
)