Apply new Standard Ensembler routing policy with Custom Experiment Engines #237

deadlycoconuts · 2022-09-09T07:04:00Z

Context

With the introduction of changes to the routing strategy of the Turing Router to support Standard Ensemblers for Custom Experiment Engines in #233, this PR introduces follow-up modifications to allow the API to set up Turing Routers that use a Standard Ensembler and a Custom Experiment Engine. The API also performs additional validation checks to ensure that any router create/update request payload does not contain both experiment_mappings and route_name_path specified in the EnsemblerStandardConfig.

Minor Fixes

This PR also includes the replacement of deprecated io/util functions from go 1.16/1.17 with their corresponding io or os counterparts:

ioutil.NopCloser -> io.NopCloser
ioutil.ReadAll -> io.ReadAll
ioutil.ReadFile -> os.ReadFile
ioutil.TempFile -> os.CreateTemp
ioutil.WriteFile -> os.WriteFile

Main Modifications

api/api/specs/routers.yaml - Addition of the route_name_path field to EnsemblerStandardConfig
api/turing/api/request/request.go - Addition of validation step to ensure ExperimentMappings and RouteNamePath are not both set at the same time
api/turing/cluster/servicebuilder/router.go - Addition of step to add route_name_path to the config map that will be used by the Turing Router upon deployment
api/turing/models/ensembler.go - Addition of RouteNamePath to the db model EnsemblerStandardConfig struct

Future Changes (Not included in this PR)

Addition of the route_name_path field to Standard Ensemblers within the Turing SDK
Addition of the new e2e test (api/e2e/test/09_deploy_router_with_route_name_path_std_ensembler_test.go) as a Turing SDK e2e test

deadlycoconuts · 2022-09-12T03:54:39Z

api/api/specs/routers.yaml

+        route_name_path:
+          type: "string"
+          pattern: '^\w*(?:\.\w+)*$'


This regex pattern here allows only period-delimited words of letters (or underscores) to be accepted as a valid route_name_path. Specifying this here also dispenses the need for manually crafting regex checks on the API for this field.

Oooh what if the nested path actually has a key with "-" in it? Shall we remove this check and keep it flexible like the TrafficRuleCondition ?

Hmm if it's just dashes we can make it ^[\w\-]*(?:\.[\w\-]+)*$ instead? Are you referring to field within TrafficRuleCondition? That one doesn't have any checks right?

Okay as discussed earlier, I'll be removing the regex changes to allow users more flexibility (at their own discretion) at specifying the route path names. Any resultant errors from any incorrect parsing of the path would be returned by the router itself.

deadlycoconuts · 2022-09-12T04:09:52Z

api/turing/testdata/cluster/servicebuilder/router_version_missing_bigquery.json

-        "image": "asia.gcr.io/gcp-project-id/echo:1.0.2",
-        "resource_request": {
-            "min_replica": 1,
-            "max_replica": 2,
-            "cpu_request": "200m",
-            "memory_request": "256Mi"
-        },
-        "endpoint": "echo?delay=10ms",
-        "timeout": "2s",
-        "port": 8080,
-        "env": [
-            {
-                "name": "TEST_ENV",
-                "value": "ensembler"
-            }
-        ]


Updated the schema to reflect the current schema of an ensembler. The previous schema was incorrect but it didn't break anything because another (earlier) section of the router config was causing the test to fail as expected.

leonlnj · 2022-09-12T09:53:18Z

api/turing/api/request/request_test.go

+		testEnsembler models.Ensembler
+		err           string
+	}{
+		"failure | both experiment mappings and route name path are set": {


maybe a positive to test case to ensure it is passing when expected to be?

krithika369

On the whole, the PR looks good! Thanks for meticulously updating all the test scenarios too. I left some small comments. Will have another quick look once they are reviewed / addressed.

api/api/specs/routers.yaml

api/e2e/test/09_deploy_router_with_route_name_path_std_ensembler_test.go

api/turing/api/request/request.go

api/turing/models/ensembler.go

...stdata/cluster/servicebuilder/router_version_success_std_ensembler_with_route_name_path.json

api/turing/validation/validator.go

deadlycoconuts · 2022-09-16T04:56:23Z

api/turing/cluster/servicebuilder/router.go

+		if ver.Ensembler.StandardConfig.ExperimentMappings != nil &&
+			len(ver.Ensembler.StandardConfig.ExperimentMappings) != 0 {
+			propsMap["experiment_mappings"] = ver.Ensembler.StandardConfig.ExperimentMappings
+		}
+		if ver.Ensembler.StandardConfig.RouteNamePath != "" {
+			propsMap["route_name_path"] = ver.Ensembler.StandardConfig.RouteNamePath
+		}


These conditional statements ensure that only non-nil/non-empty values are passed to the fiber config.

krithika369

LGTM! Thanks for the changes.

Since we haven't yet added the route_name_path to the SDK, can we do that in a separate PR? https://github.com/caraml-dev/turing/blob/main/sdk/turing/router/config/router_ensembler_config.py#L35

api/turing/validation/validator.go

…d route name path are not both set

…nly when docker/pyfunc types are used

…lers with route name path

…oute name path

…ion layer

…validation checks

deadlycoconuts · 2022-09-19T02:59:26Z

Thanks a lot for reviewing the PR and for all the comments! I'll be merging this right now and will be following up with a separate PR for the SDK-related changes and e2e tests! 🚀🚀

deadlycoconuts self-assigned this Sep 9, 2022

deadlycoconuts force-pushed the apply_std_ensembler_routing_policy_with_router_deployments branch from f539d5f to 1efe78c Compare September 9, 2022 09:00

deadlycoconuts changed the title ~~Apply new Standard Ensembler routing policy with router deployments~~ Apply new Standard Ensembler routing policy with Custom Experiment Engines Sep 12, 2022

deadlycoconuts commented Sep 12, 2022

View reviewed changes

deadlycoconuts requested a review from a team September 12, 2022 06:11

deadlycoconuts marked this pull request as ready for review September 12, 2022 06:11

leonlnj reviewed Sep 12, 2022

View reviewed changes

deadlycoconuts marked this pull request as draft September 12, 2022 10:04

deadlycoconuts force-pushed the apply_std_ensembler_routing_policy_with_router_deployments branch 6 times, most recently from 755d8f1 to 7f1c050 Compare September 13, 2022 06:22

deadlycoconuts marked this pull request as ready for review September 13, 2022 07:54

deadlycoconuts force-pushed the apply_std_ensembler_routing_policy_with_router_deployments branch from 06355c0 to 12b24e1 Compare September 13, 2022 10:15

krithika369 reviewed Sep 14, 2022

View reviewed changes

deadlycoconuts force-pushed the apply_std_ensembler_routing_policy_with_router_deployments branch 7 times, most recently from c4668b8 to 9ef2e8b Compare September 16, 2022 04:17

deadlycoconuts commented Sep 16, 2022

View reviewed changes

api/turing/validation/validator.go Show resolved Hide resolved

deadlycoconuts commented Sep 16, 2022

View reviewed changes

krithika369 approved these changes Sep 17, 2022

View reviewed changes

api/turing/validation/validator.go Show resolved Hide resolved

deadlycoconuts added 2 commits September 19, 2022 08:58

Add route_name_path to standard ensembler config

c10fb39

Update ensembler struct in test data to reflect latest schema

6c9f77c

deadlycoconuts added 23 commits September 19, 2022 08:58

Add additional validation tests to ensure that experiment mappings an…

519aa4e

…d route name path are not both set

Add additional test case to verify the route name path is valid

a759599

Refactor request parsing test

8f6738c

Remove redundant regex route path name check

2947074

Update regex expression to allow empty route_name_path string

8e54481

Remove required tag from EnsemblerStandardConfig.RouteNamePath

377ae04

Add e2e tests for std ensemblers with route_name_path

39223e3

Add additional conditional to e2e tests to check for ensembler pods o…

c35486f

…nly when docker/pyfunc types are used

Fix e2e test data

4449d2d

Add positive test to verify valid router config creation

382a18e

Fix router api test data

51eb7a2

Add additional unit tests for router service creation with std ensemb…

5efd62f

…lers with route name path

Replace deprecated ioutil functions with corresponding io or os ones

1af930b

Fix lint comments on indenting conditional statements

32666cd

Fix e2e tests after changes from rebasing

38b1c7c

Fix bug in e2e test for std ensembler with route name path

47f95ef

Fix sdk e2e tests

d93c4db

Remove experiment mappings as a required field and update regex for r…

b1916d7

…oute name path

Add custom validator for standard ensembler config

73b5f04

Remove standard ensembler config schema checks from request verificat…

ce78817

…ion layer

Make standard ensembler only parse non nil configs to fiber config

25f6039

Remove unnecessary route name path field in json request

673d7ab

Remove duplicated error messages from EnsemblerStandardConfig custom …

c0c66db

…validation checks

deadlycoconuts force-pushed the apply_std_ensembler_routing_policy_with_router_deployments branch from d397131 to c0c66db Compare September 19, 2022 00:59

Remove regex expression for route_name_path

0d79a22

deadlycoconuts merged commit 9aedab8 into caraml-dev:main Sep 19, 2022

deadlycoconuts deleted the apply_std_ensembler_routing_policy_with_router_deployments branch September 19, 2022 03:00

This was referenced Sep 21, 2022

Configure Turing UI to use Standard Ensembler for Custom Experiment Engines #235

Merged

Add Standard Ensembler with Route Name Path to SDK #248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply new Standard Ensembler routing policy with Custom Experiment Engines #237

Apply new Standard Ensembler routing policy with Custom Experiment Engines #237

deadlycoconuts commented Sep 9, 2022 •

edited

Loading

deadlycoconuts Sep 12, 2022

krithika369 Sep 14, 2022

deadlycoconuts Sep 14, 2022

deadlycoconuts Sep 19, 2022

deadlycoconuts Sep 12, 2022

leonlnj Sep 12, 2022

krithika369 left a comment

deadlycoconuts Sep 16, 2022 •

edited

Loading

krithika369 left a comment

deadlycoconuts commented Sep 19, 2022

Apply new Standard Ensembler routing policy with Custom Experiment Engines #237

Apply new Standard Ensembler routing policy with Custom Experiment Engines #237

Conversation

deadlycoconuts commented Sep 9, 2022 • edited Loading

Context

Minor Fixes

Main Modifications

Future Changes (Not included in this PR)

deadlycoconuts Sep 12, 2022

Choose a reason for hiding this comment

krithika369 Sep 14, 2022

Choose a reason for hiding this comment

deadlycoconuts Sep 14, 2022

Choose a reason for hiding this comment

deadlycoconuts Sep 19, 2022

Choose a reason for hiding this comment

deadlycoconuts Sep 12, 2022

Choose a reason for hiding this comment

leonlnj Sep 12, 2022

Choose a reason for hiding this comment

krithika369 left a comment

Choose a reason for hiding this comment

deadlycoconuts Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

krithika369 left a comment

Choose a reason for hiding this comment

deadlycoconuts commented Sep 19, 2022

deadlycoconuts commented Sep 9, 2022 •

edited

Loading

deadlycoconuts Sep 16, 2022 •

edited

Loading