FeatureStore, FeatureView, Config, and BIgQuerySource classes for updated SDK #1364
Conversation
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
jklegar
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
|
/kind feature |
sdk/python/feast/big_query_source.py
Outdated
| field_mapping: Optional[Dict[str, str]], | ||
| query: Optional[str], | ||
| ): | ||
| assert (table_ref is None) != (query is None), "Exactly one of table_ref and query should be specified" |
There was a problem hiding this comment.
Small nitpick, but generally I think we should avoid using assert for errors that we expect users will make in lieu of normal exceptions
Assert should normally be used for impossible to reach situations.
sdk/python/feast/feature_store.py
Outdated
| self.config = Config.from_config_path(config_path) | ||
| else: | ||
| self.config = Config() | ||
| return |
| ): | ||
| self.provider = provider if (provider is not None) else "local" | ||
| self.online_store = online_store if (online_store is not None) else "local" | ||
| self.metadata_store = metadata_store if (metadata_store is not None) else join(expanduser("~"), ".feast", "metadata_store") |
There was a problem hiding this comment.
Would it make sense to rather use the project repository as the local metadata store location? Using a shared folder across multiple projects seems like it can cause problems.
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
|
|
||
| class Config: | ||
| """ | ||
| Python representation of the FeatureStore config that the user can specify via a yaml file. |
There was a problem hiding this comment.
Nit: If the user doesnt know what the yaml file is then this comment doesnt add value. How about Configuration object that contains all possible configuration options for a FeatureStore?
sdk/python/feast/feature_view.py
Outdated
|
|
||
| class FeatureView: | ||
| """ | ||
| Represents a collection of features that will be served online. |
There was a problem hiding this comment.
We could in theory also have batch only users. How about A FeatureView defines a logical grouping of servable features?
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
| ) | ||
|
|
||
| @classmethod | ||
| def from_config_path(cls, config_path: str): |
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jklegar, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
Signed-off-by: Jacob Klegar jacob@tecton.ai
What this PR does / why we need it: Scaffold classes for updated SDK; FeatureStore will replace the Client class but these are currently just mostly-empty classes.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: