-
Notifications
You must be signed in to change notification settings - Fork 996
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
Refactor OnlineStoreConfig classes into owning modules #1649
Changes from all commits
fe441c1
f79a3b9
587b981
b9bc19b
37b5dd8
fa5fd81
32dce48
1060170
39be498
25c9a45
3ee9185
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,15 +42,15 @@ def __init__(self, provider_name): | |
super().__init__(f"Provider '{provider_name}' is not implemented") | ||
|
||
|
||
class FeastProviderModuleImportError(Exception): | ||
def __init__(self, module_name): | ||
super().__init__(f"Could not import provider module '{module_name}'") | ||
class FeastModuleImportError(Exception): | ||
def __init__(self, module_name, module_type="provider"): | ||
super().__init__(f"Could not import {module_type} module '{module_name}'") | ||
|
||
|
||
class FeastProviderClassImportError(Exception): | ||
def __init__(self, module_name, class_name): | ||
class FeastClassImportError(Exception): | ||
def __init__(self, module_name, class_name, class_type="provider"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in #1657 |
||
super().__init__( | ||
f"Could not import provider '{class_name}' from module '{module_name}'" | ||
f"Could not import {class_type} '{class_name}' from module '{module_name}'" | ||
) | ||
|
||
|
||
|
@@ -71,6 +71,20 @@ def __init__(self, offline_store_name: str, data_source_name: str): | |
) | ||
|
||
|
||
class FeastOnlineStoreInvalidName(Exception): | ||
def __init__(self, online_store_class_name: str): | ||
super().__init__( | ||
f"Online Store Class '{online_store_class_name}' should end with the string `OnlineStore`.'" | ||
) | ||
|
||
|
||
class FeastOnlineStoreConfigInvalidName(Exception): | ||
def __init__(self, online_store_config_class_name: str): | ||
super().__init__( | ||
f"Online Store Config Class '{online_store_config_class_name}' should end with the string `OnlineStoreConfig`.'" | ||
) | ||
|
||
|
||
class FeastOnlineStoreUnsupportedDataSource(Exception): | ||
def __init__(self, online_store_name: str, data_source_name: str): | ||
super().__init__( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,65 +1,41 @@ | ||
import importlib | ||
import struct | ||
from typing import Any, Dict, Set | ||
from typing import Any | ||
|
||
import mmh3 | ||
|
||
from feast.data_source import BigQuerySource, DataSource, FileSource | ||
from feast.errors import FeastOnlineStoreUnsupportedDataSource | ||
from feast import errors | ||
from feast.infra.online_stores.online_store import OnlineStore | ||
from feast.protos.feast.storage.Redis_pb2 import RedisKeyV2 as RedisKeyProto | ||
from feast.protos.feast.types.EntityKey_pb2 import EntityKey as EntityKeyProto | ||
from feast.repo_config import ( | ||
DatastoreOnlineStoreConfig, | ||
OnlineStoreConfig, | ||
RedisOnlineStoreConfig, | ||
SqliteOnlineStoreConfig, | ||
) | ||
|
||
|
||
def get_online_store_from_config( | ||
online_store_config: OnlineStoreConfig, | ||
) -> OnlineStore: | ||
def get_online_store_from_config(online_store_config: Any,) -> OnlineStore: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: trailing comma There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the type of online_store_config be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has to be |
||
"""Get the offline store from offline store config""" | ||
|
||
if isinstance(online_store_config, SqliteOnlineStoreConfig): | ||
from feast.infra.online_stores.sqlite import SqliteOnlineStore | ||
|
||
return SqliteOnlineStore() | ||
elif isinstance(online_store_config, DatastoreOnlineStoreConfig): | ||
from feast.infra.online_stores.datastore import DatastoreOnlineStore | ||
|
||
return DatastoreOnlineStore() | ||
elif isinstance(online_store_config, RedisOnlineStoreConfig): | ||
from feast.infra.online_stores.redis import RedisOnlineStore | ||
|
||
return RedisOnlineStore() | ||
raise ValueError(f"Unsupported offline store config '{online_store_config}'") | ||
|
||
|
||
SUPPORTED_SOURCES: Dict[Any, Set[Any]] = { | ||
SqliteOnlineStoreConfig: {FileSource}, | ||
DatastoreOnlineStoreConfig: {BigQuerySource}, | ||
RedisOnlineStoreConfig: {FileSource, BigQuerySource}, | ||
} | ||
|
||
|
||
def assert_online_store_supports_data_source( | ||
online_store_config: OnlineStoreConfig, data_source: DataSource | ||
): | ||
supported_sources: Set[Any] = SUPPORTED_SOURCES.get( | ||
online_store_config.__class__, set() | ||
) | ||
# This is needed because checking for `in` with Union types breaks mypy. | ||
# https://github.com/python/mypy/issues/4954 | ||
# We can replace this with `data_source.__class__ in SUPPORTED_SOURCES[online_store_config.__class__]` | ||
# Once ^ is resolved. | ||
if supported_sources: | ||
for source in supported_sources: | ||
if source == data_source.__class__: | ||
return | ||
raise FeastOnlineStoreUnsupportedDataSource( | ||
online_store_config.type, data_source.__class__.__name__ | ||
) | ||
module_name = online_store_config.__module__ | ||
qualified_name = type(online_store_config).__name__ | ||
store_class_name = qualified_name.replace("Config", "") | ||
try: | ||
module = importlib.import_module(module_name) | ||
except Exception as e: | ||
# The original exception can be anything - either module not found, | ||
# or any other kind of error happening during the module import time. | ||
# So we should include the original error as well in the stack trace. | ||
raise errors.FeastModuleImportError( | ||
module_name, module_type="OnlineStore" | ||
) from e | ||
|
||
# Try getting the provider class definition | ||
try: | ||
online_store_class = getattr(module, store_class_name) | ||
except AttributeError: | ||
# This can only be one type of error, when class_name attribute does not exist in the module | ||
# So we don't have to include the original exception here | ||
raise errors.FeastClassImportError( | ||
module_name, store_class_name, class_type="OnlineStore" | ||
) from None | ||
return online_store_class() | ||
|
||
|
||
def _redis_key(project: str, entity_key: EntityKeyProto): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove the default module_type here, instead explicitly pass the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in #1657