From 4dce254dc8c4f7de0c6005907ceba53b44f264ce Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 18:13:38 -0700 Subject: [PATCH] fix: Enforce kw args featureservice (#2575) * Fix Signed-off-by: Kevin Zhang * Fix lint Signed-off-by: Kevin Zhang * Fix Signed-off-by: Kevin Zhang * Fix lint Signed-off-by: Kevin Zhang * Add tests Signed-off-by: Kevin Zhang * Fix lint Signed-off-by: Kevin Zhang * Fix Signed-off-by: Kevin Zhang * Fix lint Signed-off-by: Kevin Zhang --- sdk/python/feast/feature_service.py | 38 +++++++++++++-- sdk/python/tests/unit/test_feature_service.py | 48 +++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/sdk/python/feast/feature_service.py b/sdk/python/feast/feature_service.py index 2febad3b1b..492d31a809 100644 --- a/sdk/python/feast/feature_service.py +++ b/sdk/python/feast/feature_service.py @@ -1,3 +1,4 @@ +import warnings from datetime import datetime from typing import Dict, List, Optional, Union @@ -47,8 +48,9 @@ class FeatureService: @log_exceptions def __init__( self, - name: str, - features: List[Union[FeatureView, OnDemandFeatureView]], + *args, + name: Optional[str] = None, + features: Optional[List[Union[FeatureView, OnDemandFeatureView]]] = None, tags: Dict[str, str] = None, description: str = "", owner: str = "", @@ -59,10 +61,38 @@ def __init__( Raises: ValueError: If one of the specified features is not a valid type. """ - self.name = name + positional_attributes = ["name", "features"] + _name = name + _features = features + if args: + warnings.warn( + ( + "Feature service parameters should be specified as a keyword argument instead of a positional arg." + "Feast 0.23+ will not support positional arguments to construct feature service" + ), + DeprecationWarning, + ) + if len(args) > len(positional_attributes): + raise ValueError( + f"Only {', '.join(positional_attributes)} are allowed as positional args when defining " + f"feature service, for backwards compatibility." + ) + if len(args) >= 1: + _name = args[0] + if len(args) >= 2: + _features = args[1] + + if not _name: + raise ValueError("Feature service name needs to be specified") + + if not _features: + # Technically, legal to create feature service with no feature views before. + _features = [] + + self.name = _name self.feature_view_projections = [] - for feature_grouping in features: + for feature_grouping in _features: if isinstance(feature_grouping, BaseFeatureView): self.feature_view_projections.append(feature_grouping.projection) else: diff --git a/sdk/python/tests/unit/test_feature_service.py b/sdk/python/tests/unit/test_feature_service.py index 80445299f2..fc4fd70bcb 100644 --- a/sdk/python/tests/unit/test_feature_service.py +++ b/sdk/python/tests/unit/test_feature_service.py @@ -1,3 +1,5 @@ +import pytest + from feast.feature_service import FeatureService from feast.feature_view import FeatureView from feast.field import Field @@ -55,3 +57,49 @@ def test_hash(): s4 = {feature_service_1, feature_service_2, feature_service_3, feature_service_4} assert len(s4) == 3 + + +def test_feature_view_kw_args_warning(): + with pytest.warns(DeprecationWarning): + service = FeatureService("name", [], tags={"tag_1": "tag"}, description="desc") + assert service.name == "name" + assert service.tags == {"tag_1": "tag"} + assert service.description == "desc" + + # More positional args than name and features + with pytest.raises(ValueError): + service = FeatureService("name", [], {"tag_1": "tag"}, "desc") + + # No name defined. + with pytest.raises(ValueError): + service = FeatureService(features=[], tags={"tag_1": "tag"}, description="desc") + + +def no_warnings(func): + def wrapper_no_warnings(*args, **kwargs): + with pytest.warns(None) as warnings: + func(*args, **kwargs) + + if len(warnings) > 0: + raise AssertionError( + "Warnings were raised: " + ", ".join([str(w) for w in warnings]) + ) + + return wrapper_no_warnings + + +@no_warnings +def test_feature_view_kw_args_normal(): + file_source = FileSource(name="my-file-source", path="test.parquet") + feature_view = FeatureView( + name="my-feature-view", + entities=[], + schema=[ + Field(name="feature1", dtype=Float32), + Field(name="feature2", dtype=Float32), + ], + source=file_source, + ) + _ = FeatureService( + name="my-feature-service", features=[feature_view[["feature1", "feature2"]]] + )