From c5ddffeb08f0752240f84a93c54332bca58c52bc Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 12:16:32 -0700 Subject: [PATCH 1/8] Fix Signed-off-by: Kevin Zhang --- sdk/python/feast/feature_service.py | 35 ++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/sdk/python/feast/feature_service.py b/sdk/python/feast/feature_service.py index 2febad3b1b..ffa522f7a4 100644 --- a/sdk/python/feast/feature_service.py +++ b/sdk/python/feast/feature_service.py @@ -1,6 +1,6 @@ from datetime import datetime from typing import Dict, List, Optional, Union - +import warnings from google.protobuf.json_format import MessageToJson from feast.base_feature_view import BaseFeatureView @@ -47,8 +47,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 +60,34 @@ 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") + + 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: From bcb78ae990fe554fb30373d69c05ddc9fae97e42 Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 12:16:57 -0700 Subject: [PATCH 2/8] Fix lint Signed-off-by: Kevin Zhang --- sdk/python/feast/feature_service.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/python/feast/feature_service.py b/sdk/python/feast/feature_service.py index ffa522f7a4..30c2f4073c 100644 --- a/sdk/python/feast/feature_service.py +++ b/sdk/python/feast/feature_service.py @@ -1,6 +1,7 @@ +import warnings from datetime import datetime from typing import Dict, List, Optional, Union -import warnings + from google.protobuf.json_format import MessageToJson from feast.base_feature_view import BaseFeatureView From 214a6d14aa1bbe6798228382f19a9338ba3dd790 Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 12:18:21 -0700 Subject: [PATCH 3/8] Fix Signed-off-by: Kevin Zhang --- sdk/python/feast/feature_service.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sdk/python/feast/feature_service.py b/sdk/python/feast/feature_service.py index 30c2f4073c..e2d4c9e2c9 100644 --- a/sdk/python/feast/feature_service.py +++ b/sdk/python/feast/feature_service.py @@ -85,6 +85,9 @@ def __init__( if not _name: raise ValueError("Feature service name needs to be specified") + if not _features: + _features = [] + self.name = _name self.feature_view_projections = [] From 881c8dbbb02d972c17175d8d8b35825fcc550cb8 Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 12:21:20 -0700 Subject: [PATCH 4/8] Fix lint Signed-off-by: Kevin Zhang --- sdk/python/feast/feature_service.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/python/feast/feature_service.py b/sdk/python/feast/feature_service.py index e2d4c9e2c9..492d31a809 100644 --- a/sdk/python/feast/feature_service.py +++ b/sdk/python/feast/feature_service.py @@ -86,6 +86,7 @@ def __init__( 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 From 1a1878d3f927c22e8e0bab1fa26bff996c496b63 Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 13:33:05 -0700 Subject: [PATCH 5/8] Add tests Signed-off-by: Kevin Zhang --- sdk/python/tests/unit/test_feature_service.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/sdk/python/tests/unit/test_feature_service.py b/sdk/python/tests/unit/test_feature_service.py index 80445299f2..ac3fd36677 100644 --- a/sdk/python/tests/unit/test_feature_service.py +++ b/sdk/python/tests/unit/test_feature_service.py @@ -3,6 +3,7 @@ from feast.field import Field from feast.infra.offline_stores.file_source import FileSource from feast.types import Float32 +import pytest def test_feature_service_with_description(): @@ -55,3 +56,26 @@ 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(): + # source_class = request.param + 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" + ) From 3b40bd9cebc3dfae3442e87501a80a8a6547ed1b Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 13:35:03 -0700 Subject: [PATCH 6/8] Fix lint Signed-off-by: Kevin Zhang --- sdk/python/tests/unit/test_feature_service.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/sdk/python/tests/unit/test_feature_service.py b/sdk/python/tests/unit/test_feature_service.py index ac3fd36677..f1280603c9 100644 --- a/sdk/python/tests/unit/test_feature_service.py +++ b/sdk/python/tests/unit/test_feature_service.py @@ -1,9 +1,10 @@ +import pytest + from feast.feature_service import FeatureService from feast.feature_view import FeatureView from feast.field import Field from feast.infra.offline_stores.file_source import FileSource from feast.types import Float32 -import pytest def test_feature_service_with_description(): @@ -61,21 +62,15 @@ def test_hash(): def test_feature_view_kw_args_warning(): # source_class = request.param with pytest.warns(DeprecationWarning): - service = FeatureService( - "name", [], tags = {"tag_1": "tag"}, description="desc" - ) + 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" - ) + service = FeatureService("name", [], {"tag_1": "tag"}, "desc") # No name defined. with pytest.raises(ValueError): - service = FeatureService( - features=[], tags={"tag_1": "tag"}, description="desc" - ) + service = FeatureService(features=[], tags={"tag_1": "tag"}, description="desc") From d807489fcef3ac18e5bd91b1d73516e7c8469e8b Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 17:41:26 -0700 Subject: [PATCH 7/8] Fix Signed-off-by: Kevin Zhang --- sdk/python/tests/unit/test_feature_service.py | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/sdk/python/tests/unit/test_feature_service.py b/sdk/python/tests/unit/test_feature_service.py index f1280603c9..24d148d0f8 100644 --- a/sdk/python/tests/unit/test_feature_service.py +++ b/sdk/python/tests/unit/test_feature_service.py @@ -60,7 +60,6 @@ def test_hash(): def test_feature_view_kw_args_warning(): - # source_class = request.param with pytest.warns(DeprecationWarning): service = FeatureService("name", [], tags={"tag_1": "tag"}, description="desc") assert service.name == "name" @@ -74,3 +73,31 @@ def test_feature_view_kw_args_warning(): # 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"]]] + ) From cf1dcf948851e53455bbcf49472b4ae8a53c0a6f Mon Sep 17 00:00:00 2001 From: Kevin Zhang Date: Tue, 19 Apr 2022 17:45:52 -0700 Subject: [PATCH 8/8] Fix lint Signed-off-by: Kevin Zhang --- sdk/python/tests/unit/test_feature_service.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/python/tests/unit/test_feature_service.py b/sdk/python/tests/unit/test_feature_service.py index 24d148d0f8..fc4fd70bcb 100644 --- a/sdk/python/tests/unit/test_feature_service.py +++ b/sdk/python/tests/unit/test_feature_service.py @@ -74,6 +74,7 @@ def test_feature_view_kw_args_warning(): 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: @@ -86,6 +87,7 @@ def wrapper_no_warnings(*args, **kwargs): return wrapper_no_warnings + @no_warnings def test_feature_view_kw_args_normal(): file_source = FileSource(name="my-file-source", path="test.parquet")