From 160d7b71a74531909fab575cb263d02b3996fac2 Mon Sep 17 00:00:00 2001
From: Kevin Zhang <kzhang@tecton.ai>
Date: Tue, 19 Apr 2022 18:13:38 -0700
Subject: [PATCH] fix: Enforce kw args featureservice (#2575)

* Fix

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

* Fix lint

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

* Fix

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

* Fix lint

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

* Add tests

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

* Fix lint

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

* Fix

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

* Fix lint

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>
---
 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"]]]
+    )