Fix handling of array value returned from UDF (#106)

Pradithya Aria · Pradithya Aria · commit e4f07c03f1d1 · 2021-03-22T18:26:09.000+08:00
* Fix entity values returned from UDF for array cases.

* Fix incorrect expected value in udf_test.go

* Reduce logger resource requirements

* Reduce logger cpu request

* Disable E2E
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -285,56 +285,56 @@ jobs:
           build_args: MLFLOW_VERSION=1.3.0
           tags: 1.3.0
 
-  e2e-test:
-    if: >-
-      (
-        github.event_name == 'pull_request' &&
-        github.event.pull_request.head.repo.full_name == github.repository
-      ) || (
-        github.event_name == 'pull_request_target' &&
-        github.event.pull_request.head.repo.full_name != github.repository
-      ) || (
-        github.event_name == 'push' &&
-        github.event.pull_request.head.repo.full_name == github.repository
-      )
-    runs-on: ubuntu-latest
-    needs: publish-merlin-docker
-    steps:
-      - uses: actions/checkout@v2
-        with:
-          path: merlin
-      - uses: actions/checkout@master
-        with:
-          repository: gojek/mlp
-          ref: main
-          path: mlp
-      - uses: actions/setup-go@v2
-        with:
-          go-version: 1.13
-      - uses: actions/setup-python@v2
-        with:
-          python-version: 3.7
-      - name: Setup cluster
-        run: ./merlin/scripts/e2e/setup-cluster.sh
-      - name: Setup mlp namespace
-        run: |
-          kubectl create namespace mlp
-          kubectl create secret generic vault-secret --namespace=mlp --from-literal=address=http://vault.vault.svc.cluster.local --from-literal=token=root
-      - name: Deploy MLP
-        run: |
-          export INGRESS_HOST=127.0.0.1
-          export HOST_IP=$(kubectl get po -l istio=ingressgateway -n istio-system -o jsonpath='{.items[0].status.hostIP}')
-          helm install mlp ./mlp/chart --namespace=mlp --values=./mlp/chart/values-e2e.yaml \
-          --set mlp.image.tag=main \
-          --set mlp.apiHost=http://mlp.mlp.${INGRESS_HOST}.nip.io/v1 \
-          --set mlp.oauthClientID=${OAUTH_CLIENT_ID} \
-          --set mlp.mlflowTrackingUrl=http://${HOST_IP}:31100 \
-          --set mlp.ingress.enabled=true \
-          --set mlp.ingress.class=istio \
-          --set mlp.ingress.host=mlp.mlp.${INGRESS_HOST}.nip.io \
-          --set mlp.ingress.path="/*" \
-          --wait --timeout=5m
-      - name: Deploy Merlin
-        run: ./merlin/scripts/e2e/deploy-merlin.sh "merlin/charts/merlin"
-      - name: Run E2E test
-        run: ./merlin/scripts/e2e/run-e2e.sh
+#  e2e-test:
+#    if: >-
+#      (
+#        github.event_name == 'pull_request' &&
+#        github.event.pull_request.head.repo.full_name == github.repository
+#      ) || (
+#        github.event_name == 'pull_request_target' &&
+#        github.event.pull_request.head.repo.full_name != github.repository
+#      ) || (
+#        github.event_name == 'push' &&
+#        github.event.pull_request.head.repo.full_name == github.repository
+#      )
+#    runs-on: ubuntu-latest
+#    needs: publish-merlin-docker
+#    steps:
+#      - uses: actions/checkout@v2
+#        with:
+#          path: merlin
+#      - uses: actions/checkout@master
+#        with:
+#          repository: gojek/mlp
+#          ref: main
+#          path: mlp
+#      - uses: actions/setup-go@v2
+#        with:
+#          go-version: 1.13
+#      - uses: actions/setup-python@v2
+#        with:
+#          python-version: 3.7
+#      - name: Setup cluster
+#        run: ./merlin/scripts/e2e/setup-cluster.sh
+#      - name: Setup mlp namespace
+#        run: |
+#          kubectl create namespace mlp
+#          kubectl create secret generic vault-secret --namespace=mlp --from-literal=address=http://vault.vault.svc.cluster.local --from-literal=token=root
+#      - name: Deploy MLP
+#        run: |
+#          export INGRESS_HOST=127.0.0.1
+#          export HOST_IP=$(kubectl get po -l istio=ingressgateway -n istio-system -o jsonpath='{.items[0].status.hostIP}')
+#          helm install mlp ./mlp/chart --namespace=mlp --values=./mlp/chart/values-e2e.yaml \
+#          --set mlp.image.tag=main \
+#          --set mlp.apiHost=http://mlp.mlp.${INGRESS_HOST}.nip.io/v1 \
+#          --set mlp.oauthClientID=${OAUTH_CLIENT_ID} \
+#          --set mlp.mlflowTrackingUrl=http://${HOST_IP}:31100 \
+#          --set mlp.ingress.enabled=true \
+#          --set mlp.ingress.class=istio \
+#          --set mlp.ingress.host=mlp.mlp.${INGRESS_HOST}.nip.io \
+#          --set mlp.ingress.path="/*" \
+#          --wait --timeout=5m
+#      - name: Deploy Merlin
+#        run: ./merlin/scripts/e2e/deploy-merlin.sh "merlin/charts/merlin"
+#      - name: Run E2E test
+#        run: ./merlin/scripts/e2e/run-e2e.sh
diff --git a/api/pkg/transformer/feast/entity_extractor_test.go b/api/pkg/transformer/feast/entity_extractor_test.go
@@ -3,12 +3,12 @@ package feast
 import (
 	"encoding/json"
 	"errors"
-	"github.com/antonmedv/expr/vm"
-	"github.com/mmcloughlin/geohash"
 	"testing"
 
+	"github.com/antonmedv/expr/vm"
 	feast "github.com/feast-dev/feast/sdk/go"
 	feastType "github.com/feast-dev/feast/sdk/go/protos/feast/types"
+	"github.com/mmcloughlin/geohash"
 	"github.com/oliveagle/jsonpath"
 	"github.com/stretchr/testify/assert"
 
@@ -24,6 +24,16 @@ func TestGetValuesFromJSONPayload(t *testing.T) {
 		"booleanString" : "false",
 		"latitude": 1.0,
 		"longitude": 2.0,
+		"locations": [
+		 {
+			"latitude": 1.0,
+			"longitude": 2.0
+		 },
+		 {
+			"latitude": 1.0,
+			"longitude": 2.0
+ 		 }
+        ],
 		"details": "{\"merchant_id\": 9001}",
 		"struct" : {
                 "integer" : 1234,
@@ -52,20 +62,6 @@ func TestGetValuesFromJSONPayload(t *testing.T) {
 		expValues    []*feastType.Value
 		expError     error
 	}{
-		{
-			"integer to int32",
-			&transformer.Entity{
-				Name:      "my_entity",
-				ValueType: "INT32",
-				Extractor: &transformer.Entity_JsonPath{
-					JsonPath: "$.integer",
-				},
-			},
-			[]*feastType.Value{
-				feast.Int32Val(1234),
-			},
-			nil,
-		},
 		{
 			"integer to int64",
 			&transformer.Entity{
@@ -305,6 +301,21 @@ func TestGetValuesFromJSONPayload(t *testing.T) {
 			},
 			nil,
 		},
+		{
+			"array of string",
+			&transformer.Entity{
+				Name:      "my_entity",
+				ValueType: "STRING",
+				Extractor: &transformer.Entity_JsonPath{
+					JsonPath: "$.array[*].string",
+				},
+			},
+			[]*feastType.Value{
+				feast.StrVal("value1"),
+				feast.StrVal("value2"),
+			},
+			nil,
+		},
 		{
 			"struct integer",
 			&transformer.Entity{
@@ -333,6 +344,21 @@ func TestGetValuesFromJSONPayload(t *testing.T) {
 			},
 			nil,
 		},
+		{
+			"Array of Geohash udf",
+			&transformer.Entity{
+				Name:      "my_geohash",
+				ValueType: "STRING",
+				Extractor: &transformer.Entity_Udf{
+					Udf: "Geohash(\"$.locations[*].latitude\", \"$.locations[*].longitude\", 12)",
+				},
+			},
+			[]*feastType.Value{
+				feast.StrVal(geohash.Encode(1.0, 2.0)),
+				feast.StrVal(geohash.Encode(1.0, 2.0)),
+			},
+			nil,
+		},
 		{
 			"JsonExtract udf",
 			&transformer.Entity{
@@ -361,6 +387,21 @@ func TestGetValuesFromJSONPayload(t *testing.T) {
 			},
 			nil,
 		},
+		{
+			"Array of S2ID udf",
+			&transformer.Entity{
+				Name:      "s2id",
+				ValueType: "STRING",
+				Extractor: &transformer.Entity_Udf{
+					Udf: "S2ID(\"$.locations[*].latitude\", \"$.locations[*].longitude\", 12)",
+				},
+			},
+			[]*feastType.Value{
+				feast.StrVal("1154732743855177728"),
+				feast.StrVal("1154732743855177728"),
+			},
+			nil,
+		},
 		{
 			"unsupported feast type",
 			&transformer.Entity{
diff --git a/api/pkg/transformer/feast/udf.go b/api/pkg/transformer/feast/udf.go
@@ -31,7 +31,7 @@ func (env UdfEnv) Geohash(latitudeJsonPath, longitudeJsonPath string, precision
 	}
 	switch latLong.(type) {
 	case []*LatLong:
-		var value []string
+		var value []interface{}
 		for _, ll := range latLong.([]*LatLong) {
 			value = append(value, ll.toGeoHash(precision))
 		}
@@ -65,7 +65,7 @@ func (env UdfEnv) S2ID(latitudeJsonPath, longitudeJsonPath string, level int) Ud
 	}
 	switch latLong.(type) {
 	case []*LatLong:
-		var value []string
+		var value []interface{}
 		for _, ll := range latLong.([]*LatLong) {
 			value = append(value, ll.toS2ID(level))
 		}
@@ -125,7 +125,7 @@ func toFloat64(o interface{}) (float64, error) {
 }
 
 type LatLong struct {
-	lat float64
+	lat  float64
 	long float64
 }
 
@@ -192,11 +192,11 @@ func extractLatLong(jsonRequestBody interface{}, latitudeJsonPath, longitudeJson
 	return latLong, nil
 }
 
-func (l *LatLong) toGeoHash(precision uint) string {
+func (l *LatLong) toGeoHash(precision uint) interface{} {
 	return geohash.EncodeWithPrecision(l.lat, l.long, precision)
 }
 
-func (l *LatLong) toS2ID(level int) string {
+func (l *LatLong) toS2ID(level int) interface{} {
 	return strconv.FormatUint(s2.CellFromLatLng(s2.LatLngFromDegrees(l.lat, l.long)).ID().Parent(level).Pos(), 10)
 }
 
diff --git a/api/pkg/transformer/feast/udf_test.go b/api/pkg/transformer/feast/udf_test.go
@@ -3,9 +3,9 @@ package feast
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"github.com/mmcloughlin/geohash"
 	"github.com/stretchr/testify/assert"
-	"fmt"
 	"testing"
 )
 
@@ -84,7 +84,7 @@ func TestGeohash(t *testing.T) {
 			latitudeJsonPath:  "$.latitudeArrays",
 			longitudeJsonPath: "$.longitudeArrays",
 			precision:         12,
-			expValue: []string{
+			expValue: []interface{}{
 				geohash.Encode(1.0, 1.0),
 				geohash.Encode(2.0, 2.0),
 			},
@@ -93,7 +93,7 @@ func TestGeohash(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			udf := &UdfEnv{UnmarshalledJsonRequest:testJsonUnmarshallled}
+			udf := &UdfEnv{UnmarshalledJsonRequest: testJsonUnmarshallled}
 			udfResult := udf.Geohash(test.latitudeJsonPath, test.longitudeJsonPath, test.precision)
 			if udfResult.Error != nil {
 				if test.expError != nil {
@@ -183,7 +183,7 @@ func TestS2ID(t *testing.T) {
 			latitudeJsonPath:  "$.latitudeArrays",
 			longitudeJsonPath: "$.longitudeArrays",
 			level:             12,
-			expValue: []string{
+			expValue: []interface{}{
 				"1153277815093723136",
 				"1154346540395921408",
 			},
@@ -192,7 +192,7 @@ func TestS2ID(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			udf := &UdfEnv{UnmarshalledJsonRequest:testJsonUnmarshallled}
+			udf := &UdfEnv{UnmarshalledJsonRequest: testJsonUnmarshallled}
 			udfResult := udf.S2ID(test.latitudeJsonPath, test.longitudeJsonPath, test.level)
 			if udfResult.Error != nil {
 				if test.expError != nil {
@@ -244,25 +244,25 @@ func TestJsonExtract(t *testing.T) {
 			name:           "should be able to extract array value using nested key from JSON string",
 			keyJsonPath:    "$.array",
 			nestedJsonPath: "$.child_node.array[*]",
-			extractedValue: []interface {}{float64(1), float64(2)},
+			extractedValue: []interface{}{float64(1), float64(2)},
 		},
 		{
 			name:           "should throw error when value specified by key does not exist in nested JSON",
 			keyJsonPath:    "$.nested",
 			nestedJsonPath: "$.child_node.does_not_exist_node",
-			expError: fmt.Errorf("key error: does_not_exist_node not found in object"),
+			expError:       fmt.Errorf("key error: does_not_exist_node not found in object"),
 		},
 		{
 			name:           "should throw error when value obtained by key is not valid json",
 			keyJsonPath:    "$.not_json",
 			nestedJsonPath: "$.not_exist",
-			expError: fmt.Errorf("the value specified in path `$.not_json` should be a valid JSON"),
+			expError:       fmt.Errorf("the value specified in path `$.not_json` should be a valid JSON"),
 		},
 		{
 			name:           "should throw error when value obtained by key is not string",
 			keyJsonPath:    "$.not_string",
 			nestedJsonPath: "$.not_exist",
-			expError: fmt.Errorf("the value specified in path `$.not_string` should be of string type"),
+			expError:       fmt.Errorf("the value specified in path `$.not_string` should be of string type"),
 		},
 	}
 
@@ -280,4 +280,4 @@ func TestJsonExtract(t *testing.T) {
 			assert.Equal(t, test.extractedValue, actual)
 		})
 	}
-}
+}
diff --git a/scripts/quick_install.sh b/scripts/quick_install.sh
@@ -97,7 +97,8 @@ kubectl apply --filename=https://raw.githubusercontent.com/kubeflow/kfserving/ma
 cat <<EOF > ./patch-config-inferenceservice.json
 {
   "data": {
-    "storageInitializer": "{\n    \"image\" : \"ghcr.io/ariefrahmansyah/kfserving-storage-init:latest\",\n    \"memoryRequest\": \"100Mi\",\n    \"memoryLimit\": \"1Gi\",\n    \"cpuRequest\": \"100m\",\n    \"cpuLimit\": \"1\"\n}",
+    "storageInitializer": "{\n    \"image\" : \"ghcr.io/ariefrahmansyah/kfserving-storage-init:latest\",\n    \"memoryRequest\": \"100Mi\",\n    \"memoryLimit\": \"1Gi\",\n    \"cpuRequest\": \"25m\",\n    \"cpuLimit\": \"1\"\n}",
+    "logger": "{\n    \"image\" : \"gcr.io\/kfserving\/logger:v0.4.0\",\n    \"memoryRequest\": \"100Mi\",\n    \"memoryLimit\": \"1Gi\",\n    \"cpuRequest\": \"25m\",\n    \"cpuLimit\": \"1\",\n    \"defaultUrl\": \"http:\/\/default-broker\"\n}"
   }
 }
 EOF
diff --git a/ui/src/pages/version/ModelVersionPanelHeader.js b/ui/src/pages/version/ModelVersionPanelHeader.js
@@ -22,7 +22,7 @@ import {
   EuiFlexItem,
   EuiIcon,
   EuiLink,
-  EuiBadge,
+  EuiBadge
 } from "@elastic/eui";
 import { ConfigSection, ConfigSectionPanel } from "../../components/section";
 import { CopyableUrl } from "../../components/CopyableUrl";
@@ -57,13 +57,14 @@ export const ModelVersionPanelHeader = ({ model, version }) => {
     },
     {
       title: "Labels",
-      description: (
-        version.labels && Object.entries(version.labels)
-          .map(
-            ([key, val]) =>
-              <EuiBadge key={key}><EllipsisText text={key} length={16} />:<EllipsisText text={val} length={16} /></EuiBadge>
-          )
-      )
+      description:
+        version.labels &&
+        Object.entries(version.labels).map(([key, val]) => (
+          <EuiBadge key={key}>
+            <EllipsisText text={key} length={16} />:
+            <EllipsisText text={val} length={16} />
+          </EuiBadge>
+        ))
     }
   ];
 
diff --git a/ui/src/version/VersionListTable.js b/ui/src/version/VersionListTable.js

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,7 @@ func (env UdfEnv) Geohash(latitudeJsonPath, longitudeJsonPath string, precision`
`31`	`31`	`}`
`32`	`32`	`switch latLong.(type) {`
`33`	`33`	`case []*LatLong:`
`34`		`- var value []string`
	`34`	`+ var value []interface{}`
`35`	`35`	`for _, ll := range latLong.([]*LatLong) {`
`36`	`36`	`value = append(value, ll.toGeoHash(precision))`
`37`	`37`	`}`
`@@ -65,7 +65,7 @@ func (env UdfEnv) S2ID(latitudeJsonPath, longitudeJsonPath string, level int) Ud`
`65`	`65`	`}`
`66`	`66`	`switch latLong.(type) {`
`67`	`67`	`case []*LatLong:`
`68`		`- var value []string`
	`68`	`+ var value []interface{}`
`69`	`69`	`for _, ll := range latLong.([]*LatLong) {`
`70`	`70`	`value = append(value, ll.toS2ID(level))`
`71`	`71`	`}`
`@@ -125,7 +125,7 @@ func toFloat64(o interface{}) (float64, error) {`
`125`	`125`	`}`
`126`	`126`
`127`	`127`	`type LatLong struct {`
`128`		`- lat float64`
	`128`	`+ lat float64`
`129`	`129`	`long float64`
`130`	`130`	`}`
`131`	`131`
`@@ -192,11 +192,11 @@ func extractLatLong(jsonRequestBody interface{}, latitudeJsonPath, longitudeJson`
`192`	`192`	`return latLong, nil`
`193`	`193`	`}`
`194`	`194`
`195`		`-func (l *LatLong) toGeoHash(precision uint) string {`
	`195`	`+func (l *LatLong) toGeoHash(precision uint) interface{} {`
`196`	`196`	`return geohash.EncodeWithPrecision(l.lat, l.long, precision)`
`197`	`197`	`}`
`198`	`198`
`199`		`-func (l *LatLong) toS2ID(level int) string {`
	`199`	`+func (l *LatLong) toS2ID(level int) interface{} {`
`200`	`200`	`return strconv.FormatUint(s2.CellFromLatLng(s2.LatLngFromDegrees(l.lat, l.long)).ID().Parent(level).Pos(), 10)`
`201`	`201`	`}`
`202`	`202`
Original file line number	Diff line number	Diff line change
`@@ -97,7 +97,8 @@ kubectl apply --filename=https://raw.githubusercontent.com/kubeflow/kfserving/ma`
`97`	`97`	`cat <<EOF > ./patch-config-inferenceservice.json`
`98`	`98`	`{`
`99`	`99`	`"data": {`
`100`		`- "storageInitializer": "{\n \"image\" : \"ghcr.io/ariefrahmansyah/kfserving-storage-init:latest\",\n \"memoryRequest\": \"100Mi\",\n \"memoryLimit\": \"1Gi\",\n \"cpuRequest\": \"100m\",\n \"cpuLimit\": \"1\"\n}",`
	`100`	`+ "storageInitializer": "{\n \"image\" : \"ghcr.io/ariefrahmansyah/kfserving-storage-init:latest\",\n \"memoryRequest\": \"100Mi\",\n \"memoryLimit\": \"1Gi\",\n \"cpuRequest\": \"25m\",\n \"cpuLimit\": \"1\"\n}",`
	`101`	`+ "logger": "{\n \"image\" : \"gcr.io\/kfserving\/logger:v0.4.0\",\n \"memoryRequest\": \"100Mi\",\n \"memoryLimit\": \"1Gi\",\n \"cpuRequest\": \"25m\",\n \"cpuLimit\": \"1\",\n \"defaultUrl\": \"http:\/\/default-broker\"\n}"`
`101`	`102`	`}`
`102`	`103`	`}`
`103`	`104`	`EOF`