Skip to content

FeatureStore, FeatureView, Config, and BIgQuerySource classes for updated SDK - #1364

Merged
feast-ci-bot merged 5 commits into
feast-dev:masterfrom
jklegar:jacob_featureview
Mar 6, 2021
Merged

FeatureStore, FeatureView, Config, and BIgQuerySource classes for updated SDK#1364
feast-ci-bot merged 5 commits into
feast-dev:masterfrom
jklegar:jacob_featureview

Conversation

@jklegar

@jklegar jklegar commented Mar 2, 2021

Copy link
Copy Markdown
Collaborator

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?:

NONE

Signed-off-by: Jacob Klegar <jacob@tecton.ai>
@jklegar

jklegar commented Mar 2, 2021

Copy link
Copy Markdown
Collaborator Author

/kind feature

@feast-ci-bot feast-ci-bot added kind/feature New feature or request and removed needs-kind labels Mar 2, 2021
Comment thread 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"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick, but generally I think we should avoid using assert for errors that we expect users will make in lieu of normal exceptions

https://stackoverflow.com/questions/40182944/whats-the-difference-between-raise-try-and-assert#:~:text=raise%20is%20typically%20used%20when,if%20a%20condition%20is%20met.&text=If%20production%20quality%20code%20raises,ve%20got%20a%20bigger%20problem.

Assert should normally be used for impossible to reach situations.

Comment thread sdk/python/feast/feature_store.py Outdated
self.config = Config.from_config_path(config_path)
else:
self.config = Config()
return

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a return here?

Comment thread sdk/python/feast/feature_store.py
):
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")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

jklegar added 2 commits March 2, 2021 20:15
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread sdk/python/feast/feature_view.py Outdated

class FeatureView:
"""
Represents a collection of features that will be served online.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_path perhaps?

Comment thread sdk/python/feast/feature_store_config.py
Signed-off-by: Jacob Klegar <jacob@tecton.ai>
@feast-ci-bot

Copy link
Copy Markdown
Collaborator

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop

woop commented Mar 6, 2021

Copy link
Copy Markdown
Member

/lgtm

@feast-ci-bot
feast-ci-bot merged commit 9eea070 into feast-dev:master Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants