From d2166e14acec6eb633f89fbe33155531bb3ec564 Mon Sep 17 00:00:00 2001 From: Achal Shah Date: Wed, 30 Mar 2022 14:28:16 -0700 Subject: [PATCH 1/3] fix: Update feature view APIs to prefer keyword args Signed-off-by: Achal Shah --- sdk/python/feast/base_feature_view.py | 11 ++- sdk/python/feast/feature_view.py | 82 ++++++++++++++++------ sdk/python/feast/on_demand_feature_view.py | 8 ++- 3 files changed, 77 insertions(+), 24 deletions(-) diff --git a/sdk/python/feast/base_feature_view.py b/sdk/python/feast/base_feature_view.py index 609feab6414..5078d021fbe 100644 --- a/sdk/python/feast/base_feature_view.py +++ b/sdk/python/feast/base_feature_view.py @@ -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 = "", @@ -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 diff --git a/sdk/python/feast/feature_view.py b/sdk/python/feast/feature_view.py index b81c0afa510..b76ba158c7a 100644 --- a/sdk/python/feast/feature_view.py +++ b/sdk/python/feast/feature_view.py @@ -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, @@ -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 " + 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): @@ -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 @@ -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 = [] diff --git a/sdk/python/feast/on_demand_feature_view.py b/sdk/python/feast/on_demand_feature_view.py index 7d3e62a036c..483d80892e7 100644 --- a/sdk/python/feast/on_demand_feature_view.py +++ b/sdk/python/feast/on_demand_feature_view.py @@ -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: From c27cf09742a21471b0a5e5b5e1bab0ed09d992cd Mon Sep 17 00:00:00 2001 From: Achal Shah Date: Thu, 31 Mar 2022 11:07:14 -0700 Subject: [PATCH 2/3] cr Signed-off-by: Achal Shah --- sdk/python/feast/feature_view.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/python/feast/feature_view.py b/sdk/python/feast/feature_view.py index b76ba158c7a..7f5383ba9eb 100644 --- a/sdk/python/feast/feature_view.py +++ b/sdk/python/feast/feature_view.py @@ -139,7 +139,7 @@ def __init__( ) if len(args) > len(positional_attributes): raise ValueError( - f"Only {', '.join(positional_attributes)} are allowed positional args when defining " + f"Only {', '.join(positional_attributes)} are allowed as positional args when defining " f"feature views, for backwards compatibility." ) if len(args) >= 1: From e5b86be4dd82cc7c011903dbae87f067ce69ef5f Mon Sep 17 00:00:00 2001 From: Achal Shah Date: Thu, 31 Mar 2022 12:40:16 -0700 Subject: [PATCH 3/3] fix tests Signed-off-by: Achal Shah --- sdk/python/feast/feature_view.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sdk/python/feast/feature_view.py b/sdk/python/feast/feature_view.py index 7f5383ba9eb..33fe3fe49b4 100644 --- a/sdk/python/feast/feature_view.py +++ b/sdk/python/feast/feature_view.py @@ -74,7 +74,7 @@ class FeatureView(BaseFeatureView): name: str entities: List[str] - ttl: timedelta + ttl: Optional[timedelta] batch_source: DataSource stream_source: Optional[DataSource] features: List[Feature] @@ -165,10 +165,8 @@ def __init__( ), DeprecationWarning, ) - elif isinstance(_ttl, timedelta): + elif isinstance(_ttl, timedelta) or _ttl is None: self.ttl = _ttl - elif not _ttl: - self.ttl = timedelta(days=1) else: raise ValueError(f"unknown value type specified for ttl {type(_ttl)}")