Bugfix: Clear default route Id from custom ensembler configs #200

krithika369 · 2022-05-27T07:48:52Z

This MR fixes a few minor bugs on the UI / SDK.

UI

ui/src/services/router/TuringRouter.js - When a router with No Ensembler / Standard Ensembler is edited and updated to use Custom Ensembler (docker / pyfunc), the default_route_id property from the previous configs should be dropped before calling the API. (We could drop this as soon as the type of the Ensembler is changed from the UI but we don't do this for other components too, because with that approach, if the user switched back to the old type in the same edit session, we would have lost the values.)
ui/src/services/ensembler/PyFuncEnsembler.js - Fix regression around missing container config display for Pyfunc ensemblers. Since the detail views now use the parsed data, including the docker_config when parsing.

SDK

Under the hood, the Standard Ensembler's fallback_response_route_id and the No Ensembler's final_response_route_id both map to the default_route_id which will eventually be removed from the SDK. While it is being supported, we were previously not accounting for the idea that the user could directly update the default_route_id when Nop / Standard Ensemblers are in use (i.e., we were copying the ensembler configs to default_route_id on to_open_api() but would have lost the direct changes to the default_route_id). This is now overcome by letting the setter for default_route_id also update the ensembler configs. Then, upon to_open_api(), we only need to look at the ensembler's configs.

To facilitate this fix and to handle potential inconsistencies in the presence of RouterEnsemblerConfig vs one of the child classes, the setter for the ensembler now forces the configs to be of the child class types. This is the first step towards addressing this: #197 (comment)

krithika369 · 2022-05-27T07:49:47Z

sdk/turing/router/config/router_config.py

@@ -250,7 +271,7 @@ def to_open_api(self) -> OpenApiModel:
        )

    def _get_default_route_id(self):
-        default_route_id = self.default_route_id
+        default_route_id = None


By setting None here, we will be clearing this value for routers with custom ensemblers.

krithika369 · 2022-05-27T07:52:17Z

Hey @terryyylim, new unit tests for the SDK are WIP (for the from_config methods and setter for default_route_id). In the meantime, please help review the fixes when you can. Thank you!

terryyylim · 2022-05-27T08:06:08Z

sdk/turing/router/config/router_config.py

+        if isinstance(self._ensembler, RouterEnsemblerConfig):
+            if self._ensembler.type == "nop" and not isinstance(self._ensembler, NopRouterEnsemblerConfig):
+                self._ensembler = NopRouterEnsemblerConfig.from_config(self._ensembler.nop_config)
+            if self._ensembler.type == "standard" and not isinstance(self._ensembler, StandardRouterEnsemblerConfig):


Not sure if I'm missing something here, why is it 2 if conditions and then followed by elif here?

Thanks, I missed updating the second check to an elif.

terryyylim · 2022-05-27T08:17:52Z

sdk/turing/router/config/router_config.py

@@ -136,6 +142,13 @@ def default_route_id(self) -> str:
        details="Please use the ensembler properties to configure the final / fallback route.")
    def default_route_id(self, default_route_id: str):
        self._default_route_id = default_route_id
+        # User may directly modify the default_route_id property while it is deprecated.
+        # So, copy to the nop / standard ensembler if set.
+        if hasattr(self, "ensembler"):


Just curious, wouldn't if self.ensembler work? 🤔

Not on __init__ which is why I added this. During init, the default_route_id attribute is created first (followed by the ensembler which always reads the value from the default_route_id if its own value is not an Ensembler instance).

terryyylim · 2022-05-27T08:22:56Z

sdk/turing/router/config/router_ensembler_config.py

@@ -350,6 +363,22 @@ def service_account(self) -> str:
    def service_account(self, service_account: str):
        self._service_account = service_account

+    @staticmethod
+    def from_config(config: turing.generated.models.EnsemblerDockerConfig):


CMIIW, this method is not being overwritten anywhere else since this class is never used by another subclass, is there a reason why this is not a classmethod (since we're initializing the class through this call)?

I chose to use staticmethod because we don't need access to the class properties. But, as you're saying, we are in fact initializing the class through the call so it makes more sense for it to be a class method. I'll update this. Thanks!

krithika369 · 2022-05-30T00:40:17Z

@terryyylim I fixed another small UI bug and updated the PR with tests. Could you take another look? Thanks!

terryyylim · 2022-05-30T01:40:09Z

ui/src/services/router/TuringRouter.js

+        "fallback_response_route_id"
+      ];
+    } else {
+      // Docker or Pyfunc ensembler, clear the default_route_id


Would there be a need to clear pyfunc_config if the Ensembler type is Docker?

Nope. Pyfunc ensemblers may also have docker configs (if it has been built), not the other way.

terryyylim · 2022-05-30T01:41:48Z

sdk/turing/router/config/router_ensembler_config.py

@@ -350,6 +364,22 @@ def service_account(self) -> str:
    def service_account(self, service_account: str):
        self._service_account = service_account

+    @classmethod
+    def from_config(cls, config: turing.generated.models.EnsemblerDockerConfig):


nit: Missing return types for classmethods.

Cool, let fix that.

* Update workflows to use Python 3.7 for ensembler engines and sdk (#190) * Update workflows to use Python 3.7 for ensembler engines and sdk * Add none return option for config in Router SDK class * Update text display settings to display entire image name * Set container image name to overflow instead of being truncated * Include response headers in logs (#191) * Update proto and classes * Add response headers and refactor naming of response bodies * Refactor logging methods * Fix tests * Fix kafka tests * Fix kafka tests * Fix line breaks * Fix line breaks * Fix line breaks * Fix typo in error message * Refactor response fields * Refactor response headers as map of strings * Refactor tests to use map of strings to represent headers * Fix non deterministic serialisation of hashmap in tests * Refactor log handler * Remove debug statement * Refactor HTTP header formatting into a helper function * Fix kafka protobuf test to use JSON as means of comparison * Rename body in proto to response to avoid breaking changes * Refactor all tests to use response instead of body to refer to request body * Fix lint import suggestion * Remove debug statements * Rename variables in http header formatting helper function * Fix BQ marshalling issues * Fix lint import suggestion * Fix lint comments * Minor fixes for experiment engine configs in the helm chart (#193) Co-authored-by: Krithika Sundararajan <[email protected]> * Nop Ensembler Config (#192) * UI changes for nop ensembler config * Make handling of router / version status consistent * SDK changes for default route * Correct the default route id in unit tests * Add tests for the nop ensembler config * Update sample code and doc * Add PR comments Co-authored-by: Krithika Sundararajan <[email protected]> * [BUG] Fix undeploy router error (#194) * Fix bugs in sample SDK script * Refactor UndeployRouterVersion to take in cleanup flag * Update DeploymentService mock class * Fix bug in deployment controller test * Refactor if else blocks * Add tests for IsKnativeServiceInNamespace and IsSecretinNamespace * Rename test method to make it consistent with the method tested * Refactor k8s deletion methods into separate methods * Fix bug in deleting deployments and services * Fix typo in router name * Rename default route from nothing to control * Make undeploying a pending router status a cleanup job * Refactor code to use ignoreNotFound flag * Fix go mod file * Bugfix: Turing API should process experiment engine passkey only if client selection enabled (#196) * Bugfix: Passkey should not be processed if client selection disabled * Update hardcoded sample plugin to use experiment variables, consistent with the runner * Update RPC plugin example and docs * Correct numbering in doc and plugin name change * Add debug message * Update Deployment controller to consider if client selection enabled * Add another unit test case for TestIsClientSelectionEnabled Co-authored-by: Krithika Sundararajan <[email protected]> * Add Fallback Response Route Config for Standard Ensemblers (#197) * UI changes for standard ensembler Fallback response * Add validation for fallback resonse route id * Update docs for the Standard Ensembler config * Routing stragey changes for default route handling * SDK changes for fallback response route id * Amend user docs Co-authored-by: Krithika Sundararajan <[email protected]> * Remove the default route configuration (#198) * Remove unused default route property * Router: Make the default_route_id only required for DefaultTuringRoutingStrategy * Make Default Route ID optional for the Turing Router creater / update API * Update e2e tests * UI: Update router view / edit to stop handling default route explicitly * UI: Exclude routes with traffic rules in the final/fallback response options * SDK: deprecate the default_route_id config * SDK: Remove default route id from samples * Update user docs * Update OpenAPI bundle * Address PR comments Co-authored-by: Krithika Sundararajan <[email protected]> * Adding SDK support for Python 3.8 and 3.9 (#199) * Update SDK / engines to support multiple Python versions * Pin cloudpickle at 2.0.0 * Introduce Python Version on the Pyfunc ensembler config * Update SDK unit tests * Update Github workflows * Update chart values * Update docs, unit tests * Update SDK CI workflows to test on all Python versions * Pin protobuf version at 3.20.1 * Address PR comments Co-authored-by: Krithika Sundararajan <[email protected]> * Bugfix: Clear default route Id from custom ensembler configs (#200) * Miscellaneous bug fixes * Add unit tests for the SDK changes, address PR comments * Bugfix: Regression in display of container configs for Pyfunc * Add type annotation to class methods Co-authored-by: Krithika Sundararajan <[email protected]> * Update chart version for app release (#201) Co-authored-by: Krithika Sundararajan <[email protected]> * Add dynamic loading of Experiment Engine config (#202) * Add dynamic loading of exp engine config * Address PR comments * Add useEffect rerender * Address PR comments * Simplify conditional logic * Attempt to fix yarn install error Co-authored-by: Ewe Zi Yi <[email protected]> Co-authored-by: Krithika Sundararajan <[email protected]> Co-authored-by: Terence Lim <[email protected]>

* Upgrade Go version/Knative/Spark Operator/Kubernetes Client/Docker Compose for dev environment. (#183) * Bump versions for all k8s based libs * Use proper context for each scenario. * Upgrade virtual service version. * Update k3d version to kubernetes 1.22. * Upgrade Go to 1.16. * Fix linting errors. * Update k3d flags. * Upgrade knative/istio/spark-operator versions in cluster init. * Update default versions in cluster-init, change e2e test to new api * Fail fast if default environment is wrong. Extra logging. * Fix unit tests. * Addressed PR comments. * Pull requests to be run on any target branch. * Upgrade to go 1.18, upgrade linter. * Upgrade experiment and router to go 1.18. * Update PR comments. * Parameterise Go and Go Linter Versions. * Update documentation with new versions and ports. * Merge from main -> knative-upgrade branch (#203) * Update workflows to use Python 3.7 for ensembler engines and sdk (#190) * Update workflows to use Python 3.7 for ensembler engines and sdk * Add none return option for config in Router SDK class * Update text display settings to display entire image name * Set container image name to overflow instead of being truncated * Include response headers in logs (#191) * Update proto and classes * Add response headers and refactor naming of response bodies * Refactor logging methods * Fix tests * Fix kafka tests * Fix kafka tests * Fix line breaks * Fix line breaks * Fix line breaks * Fix typo in error message * Refactor response fields * Refactor response headers as map of strings * Refactor tests to use map of strings to represent headers * Fix non deterministic serialisation of hashmap in tests * Refactor log handler * Remove debug statement * Refactor HTTP header formatting into a helper function * Fix kafka protobuf test to use JSON as means of comparison * Rename body in proto to response to avoid breaking changes * Refactor all tests to use response instead of body to refer to request body * Fix lint import suggestion * Remove debug statements * Rename variables in http header formatting helper function * Fix BQ marshalling issues * Fix lint import suggestion * Fix lint comments * Minor fixes for experiment engine configs in the helm chart (#193) Co-authored-by: Krithika Sundararajan <[email protected]> * Nop Ensembler Config (#192) * UI changes for nop ensembler config * Make handling of router / version status consistent * SDK changes for default route * Correct the default route id in unit tests * Add tests for the nop ensembler config * Update sample code and doc * Add PR comments Co-authored-by: Krithika Sundararajan <[email protected]> * [BUG] Fix undeploy router error (#194) * Fix bugs in sample SDK script * Refactor UndeployRouterVersion to take in cleanup flag * Update DeploymentService mock class * Fix bug in deployment controller test * Refactor if else blocks * Add tests for IsKnativeServiceInNamespace and IsSecretinNamespace * Rename test method to make it consistent with the method tested * Refactor k8s deletion methods into separate methods * Fix bug in deleting deployments and services * Fix typo in router name * Rename default route from nothing to control * Make undeploying a pending router status a cleanup job * Refactor code to use ignoreNotFound flag * Fix go mod file * Bugfix: Turing API should process experiment engine passkey only if client selection enabled (#196) * Bugfix: Passkey should not be processed if client selection disabled * Update hardcoded sample plugin to use experiment variables, consistent with the runner * Update RPC plugin example and docs * Correct numbering in doc and plugin name change * Add debug message * Update Deployment controller to consider if client selection enabled * Add another unit test case for TestIsClientSelectionEnabled Co-authored-by: Krithika Sundararajan <[email protected]> * Add Fallback Response Route Config for Standard Ensemblers (#197) * UI changes for standard ensembler Fallback response * Add validation for fallback resonse route id * Update docs for the Standard Ensembler config * Routing stragey changes for default route handling * SDK changes for fallback response route id * Amend user docs Co-authored-by: Krithika Sundararajan <[email protected]> * Remove the default route configuration (#198) * Remove unused default route property * Router: Make the default_route_id only required for DefaultTuringRoutingStrategy * Make Default Route ID optional for the Turing Router creater / update API * Update e2e tests * UI: Update router view / edit to stop handling default route explicitly * UI: Exclude routes with traffic rules in the final/fallback response options * SDK: deprecate the default_route_id config * SDK: Remove default route id from samples * Update user docs * Update OpenAPI bundle * Address PR comments Co-authored-by: Krithika Sundararajan <[email protected]> * Adding SDK support for Python 3.8 and 3.9 (#199) * Update SDK / engines to support multiple Python versions * Pin cloudpickle at 2.0.0 * Introduce Python Version on the Pyfunc ensembler config * Update SDK unit tests * Update Github workflows * Update chart values * Update docs, unit tests * Update SDK CI workflows to test on all Python versions * Pin protobuf version at 3.20.1 * Address PR comments Co-authored-by: Krithika Sundararajan <[email protected]> * Bugfix: Clear default route Id from custom ensembler configs (#200) * Miscellaneous bug fixes * Add unit tests for the SDK changes, address PR comments * Bugfix: Regression in display of container configs for Pyfunc * Add type annotation to class methods Co-authored-by: Krithika Sundararajan <[email protected]> * Update chart version for app release (#201) Co-authored-by: Krithika Sundararajan <[email protected]> * Add dynamic loading of Experiment Engine config (#202) * Add dynamic loading of exp engine config * Address PR comments * Add useEffect rerender * Address PR comments * Simplify conditional logic * Attempt to fix yarn install error Co-authored-by: Ewe Zi Yi <[email protected]> Co-authored-by: Krithika Sundararajan <[email protected]> Co-authored-by: Terence Lim <[email protected]> * Update CI specs * Revert UI changes during merge * Update CI specs * Update e2e deployment timeout * Remove WIP inline comment Co-authored-by: Ashwin <[email protected]> Co-authored-by: Ewe Zi Yi <[email protected]> Co-authored-by: Krithika Sundararajan <[email protected]> Co-authored-by: Terence Lim <[email protected]>

Miscellaneous bug fixes

a80f91e

krithika369 commented May 27, 2022

View reviewed changes

krithika369 requested a review from terryyylim May 27, 2022 07:49

terryyylim reviewed May 27, 2022

View reviewed changes

Krithika Sundararajan added 3 commits May 28, 2022 19:21

Add unit tests for the SDK changes, address PR comments

5a4fe78

Merge branch 'main' into ui_fixes

8e93224

Bugfix: Regression in display of container configs for Pyfunc

8848625

terryyylim reviewed May 30, 2022

View reviewed changes

terryyylim approved these changes May 30, 2022

View reviewed changes

Add type annotation to class methods

fdcb586

krithika369 merged commit 7d90d0a into caraml-dev:main May 30, 2022

krithika369 deleted the ui_fixes branch May 30, 2022 02:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Clear default route Id from custom ensembler configs #200

Bugfix: Clear default route Id from custom ensembler configs #200

krithika369 commented May 27, 2022 •

edited

Loading

krithika369 May 27, 2022

krithika369 commented May 27, 2022

terryyylim May 27, 2022

krithika369 May 27, 2022

terryyylim May 27, 2022

krithika369 May 27, 2022 •

edited

Loading

terryyylim May 27, 2022

krithika369 May 27, 2022

krithika369 commented May 30, 2022

terryyylim May 30, 2022

krithika369 May 30, 2022

terryyylim May 30, 2022

krithika369 May 30, 2022

Bugfix: Clear default route Id from custom ensembler configs #200

Bugfix: Clear default route Id from custom ensembler configs #200

Conversation

krithika369 commented May 27, 2022 • edited Loading

UI

SDK

Choose a reason for hiding this comment

krithika369 commented May 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krithika369 May 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krithika369 commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krithika369 commented May 27, 2022 •

edited

Loading

krithika369 May 27, 2022 •

edited

Loading