Skip to content

fix: Update feature view APIs to prefer keyword args #2472

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.

Sign up for GitHub

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

Merged
feast-ci-bot merged 3 commits into from
Mar 31, 2022
Merged

fix: Update feature view APIs to prefer keyword args #2472

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2166e1
fix: Update feature view APIs to prefer keyword args
achals Mar 30, 2022
c27cf09
cr
achals Mar 31, 2022
e5b86be
fix tests
achals Mar 31, 2022
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
11 changes: 8 additions & 3 deletions sdk/python/feast/base_feature_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ class BaseFeatureView(ABC):
@abstractmethod
def __init__(
self,
name: str,
features: List[Feature],
*,
name: Optional[str] = None,
features: Optional[List[Feature]] = None,
description: str = "",
tags: Optional[Dict[str, str]] = None,
owner: str = "",
Expand All @@ -72,8 +73,12 @@ def __init__(
Raises:
ValueError: A field mapping conflicts with an Entity or a Feature.
"""
if not name:
raise ValueError("Name needs to be provided")
self.name = name
self.features = features

self.features = features or []

self.description = description
self.tags = tags or {}
self.owner = owner
Expand Down
82 changes: 62 additions & 20 deletions sdk/python/feast/feature_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,10 @@ class FeatureView(BaseFeatureView):
@log_exceptions
def __init__(
self,
name: str,
entities: List[str],
ttl: Union[Duration, timedelta],
*args,
name: Optional[str] = None,
entities: Optional[List[str]] = None,
ttl: Optional[Union[Duration, timedelta]] = None,
batch_source: Optional[DataSource] = None,
stream_source: Optional[DataSource] = None,
features: Optional[List[Feature]] = None,
Expand Down Expand Up @@ -121,6 +122,56 @@ def __init__(
Raises:
ValueError: A field mapping conflicts with an Entity or a Feature.
"""

positional_attributes = ["name, entities, ttl"]

_name = name
_entities = entities
_ttl = ttl

if args:
warnings.warn(
(
"feature view parameters should be specified as a keyword argument instead of a positional arg."
"Feast 0.23+ will not support positional arguments to construct feature views"
),
DeprecationWarning,
)
if len(args) > len(positional_attributes):
raise ValueError(
f"Only {', '.join(positional_attributes)} are allowed positional args when defining "
Copy link
Copy Markdown
Collaborator

@kevjumba kevjumba Mar 31, 2022

Choose a reason for hiding this comment

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

nit: allowed as positional args

f"feature views, for backwards compatibility."
)
if len(args) >= 1:
_name = args[0]
if len(args) >= 2:
_entities = args[1]
if len(args) >= 3:
_ttl = args[2]

if not _name:
raise ValueError("feature view name needs to be specified")

self.name = _name
self.entities = _entities if _entities else [DUMMY_ENTITY_NAME]

if isinstance(_ttl, Duration):
self.ttl = timedelta(seconds=int(_ttl.seconds))
warnings.warn(
(
"The option to pass a Duration object to the ttl parameter is being deprecated. "
"Please pass a timedelta object instead. Feast 0.21 and onwards will not support "
"Duration objects."
),
DeprecationWarning,
)
elif isinstance(_ttl, timedelta):
self.ttl = _ttl
elif not _ttl:
self.ttl = timedelta(days=1)
else:
raise ValueError(f"unknown value type specified for ttl {type(_ttl)}")

_features = features or []

if stream_source is not None and isinstance(stream_source, PushSource):
Expand All @@ -138,7 +189,7 @@ def __init__(
)
self.batch_source = batch_source

cols = [entity for entity in entities] + [feat.name for feat in _features]
cols = [entity for entity in self.entities] + [feat.name for feat in _features]
for col in cols:
if (
self.batch_source.field_mapping is not None
Expand All @@ -150,22 +201,13 @@ def __init__(
f"Entity or Feature name."
)

super().__init__(name, _features, description, tags, owner)
self.entities = entities if entities else [DUMMY_ENTITY_NAME]

if isinstance(ttl, Duration):
self.ttl = timedelta(seconds=int(ttl.seconds))
warnings.warn(
(
"The option to pass a Duration object to the ttl parameter is being deprecated. "
"Please pass a timedelta object instead. Feast 0.21 and onwards will not support "
"Duration objects."
),
DeprecationWarning,
)
else:
self.ttl = ttl

super().__init__(
name=name,
features=_features,
description=description,
tags=tags,
owner=owner,
)
self.online = online
self.stream_source = stream_source
self.materialization_intervals = []
Expand Down
8 changes: 7 additions & 1 deletion sdk/python/feast/on_demand_feature_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,13 @@ def __init__(
owner (optional): The owner of the on demand feature view, typically the email
of the primary maintainer.
"""
super().__init__(name, features, description, tags, owner)
super().__init__(
name=name,
features=features,
description=description,
tags=tags,
owner=owner,
)
if inputs and sources:
raise ValueError("At most one of `sources` or `inputs` can be specified.")
elif inputs:
Expand Down