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

deadlycoconuts · 2022-09-08T09:42:33Z

Context

With the changes introduced in #237 and #233 to support the usage of Standard Ensemblers with Custom Experiment Engines, the Turing UI has been updated in this PR to consume Standard Ensembler components exposed by the UI of these custom engines.

There are two new components that Turing UI will now consume - EditStandardEnsemblerConfig and StandardEnsemblerConfigDetails. As their names suggest, they are used respectively for the create/edit router form and the router details view respectively.

Changes are made to the existing StandardEnsemblerFormGroup and EnsemblerConfigSection components to either display the regular treatment mappings panel/table if the configured experiment engine is of the standard type, and an exposed UI component otherwise. In either case, the fallback route panel/config section will always be displayed in both scenarios.

Main Modifications

ui/src/components/ensembler/StandardEnsemblerLoaderComponent.js - Addition of a new loader for the Standard Ensembler components
ui/src/router/components/configuration/components/EnsemblerConfigSection.js - Refactoring of the child components to be displayed when a Standard Ensembler is selected in a single StandardConfigViewGroup component
ui/src/router/components/configuration/components/standard_config_section/StandardConfigViewGroup.js - Addition of a new component that displays either the treatment mappings view or the exposed component, depending on the experiment engine type
ui/src/router/components/form/components/ensembler_config/standard_ensembler/StandardEnsemblerFormGroup.js - Addition of extra logic to display the exposed UI component if a custom experiment engine is configured
ui/src/router/components/form/components/ensembler_config/typeOptions.js - Addition of a new description for the standard ensembler if a custom experiment engine is configured
ui/src/services/ensembler/StandardEnsembler.js - Addition of a new route_name_path field to the standard ensembler object

deadlycoconuts · 2022-09-12T10:51:57Z

ui/src/components/page_navigation/PageNavigation.js

@@ -85,7 +85,6 @@ const MoreActionsButton = ({ actions }) => {
      panelPaddingSize="none"
      anchorPosition="downRight">
      <EuiContextMenuPanel
-        hasFocus={false}


Removed this because hasFocus is no longer a prop for EuiContextMenuPanel (see https://elastic.github.io/eui/#/navigation/context-menu).

deadlycoconuts · 2022-09-21T05:16:33Z

ui/src/router/components/form/components/ensembler_config/typeOptions.js

@@ -14,13 +14,26 @@ const typeOptions = [
  {
    value: "standard",
    inputDisplay: "Standard",
+    expenginetype: "standard",


Added this field so the function ensemblerTypeOptions is able to distinguish between the two typeOptions that both have the value "standard".

deadlycoconuts · 2022-09-21T05:25:30Z

ui/src/router/components/form/components/ensembler_config/typeOptions.js

-      ?.experiment_selection_enabled
-  ) {
+  const ensemblerOptions = typeOptions.filter(
+    (o) => o.value !== "nop" && (o.expenginetype === engineProps.type || o.expenginetype === undefined)


This additional condition selects the standard ensembler typeOption with the correct expenginetype. If a typeOption does not have the expenginetype field set (which implicitly means it's not a standard ensembler), then it will not be removed.

Note: Just noticed the 2 subconditions here (o.expenginetype === engineProps.type || o.expenginetype === undefined) can be inverted for slightly better computation speed 😅

deadlycoconuts · 2022-09-21T05:34:14Z

ui/src/router/components/form/components/ensembler_config/typeOptions.js

+  const ensemblerOptions = typeOptions.filter(
+    (o) => o.value !== "nop" && (o.expenginetype === engineProps.type || o.expenginetype === undefined)
+  );
+  if (engineProps?.standard_experiment_manager_config?.experiment_selection_enabled === false) {


This new condition (compared with (!engineProps?.standard_experiment_manager_config?.experiment_selection_enabled) previously) ensures that the experiment manager config has to explicitly disallow experiment selection before we remove the standard ensembler option (i.e. standard ensemblers will be selectable with custom experiment engines).

This contrasts with the previous condition which can return true whenever the engineProps does not have have the standard_experiment_manager_config.experiment_selection_enabled path defined (i.e. standard ensemblers are not selectable with custom experiment engines).

deadlycoconuts · 2022-09-21T05:35:00Z

ui/src/router/components/form/components/ensembler_config/typeOptions.js

+      <Fragment>
+        Turing will select the route response corresponding to the route name specified
+        in the treatment configuration. The route name path will be used to locate the
+        name of the route within the treatment configuration.
+      </Fragment>


Added a separate standard ensembler description for custom experiment engines.

Do we have to introduce this? The existing description seems generic enough to conceptually cover both cases, no?

Turing will select the response from one of the routes, based on the configured mapping between routes and experiment treatments

It may also help simplify some of the implementation (like https://github.com/caraml-dev/turing/pull/235/files#r976051034)

Well, I was originally thinking that "configured mapping between routes and experiment treatments" doesn't accurately describe whatever we're going to display in the standard ensembler configuration panel, i.e. the "assigned routes" and "assigned experiments" (there's no mention of "treatment" anywhere).

But technically, since we are expecting the experiment engines to implement the standard ensembler component to be consumed, I guess it makes sense that we should probably keep this description as generic as possible, and then expect more tooltips/information within the consumed component to explain what's going on.

Yes, I think we can keep this description generic. Any additional details specific to the std / custom engines can be embedded within those components (like the Flyout we have for XP's remote component for the std ensembler). That way we can do away with the special handling based on expenginetype introduced in this file.

Okay done, I've removed the standard ensembler option for custom experiment engines so it's now the same as before :)

terryyylim

LGTM on the whole, left some comments to address, thanks @deadlycoconuts !

terryyylim · 2022-09-22T07:42:57Z

ui/src/components/ensembler/StandardEnsemblerLoaderComponent.js

+};
+
+// Dynamic Script Loading component wrapper
+export const StandardEnsemblerLoaderComponent = ({


I think there's room to combine this component with the existing ExperimentEngineLoaderComponent. The only real difference is the props naming for remoteUi vs. experimentEngine, even the LoadDynamicScript is the same. Perhaps, we can shift this file to src/components/remote_component now that it is used in both ensembler and experiments' related components?

Okay done, now both of those components got reduced to a single RemoteLoaderComponent :)

terryyylim · 2022-09-22T07:44:25Z

...outer/components/configuration/components/standard_config_section/StandardConfigViewGroup.js

+  return (
+    !!standardConfig && (
+      <EuiFlexGroup direction="column">
+        <>


I think this is not required.

Thanks for catching that 😅

terryyylim · 2022-09-22T07:51:42Z

ui/src/router/components/form/components/ensembler_config/typeOptions.js

@@ -14,13 +14,26 @@ const typeOptions = [
  {
    value: "standard",
    inputDisplay: "Standard",
+    expenginetype: "standard",


nit: camelCase here so expEngineType

Hmm the reason why I left this as entirely lowercase was because the items in typeOptions do not get the expEngineType field stripped off when typeOptions gets passed into EuiSuperSelect as a prop, since expEngineType is a completely custom field not recognised by EUI. What then happens is that these items get rendered as DOM elements, with the expEngineType field being treated as a prop and causing this warning:

An alternative to avoiding this is to consciously strip the typeOptions of the expEngineType field every single time they are being used but I find this harder to maintain and to enforce.

I see, thanks for clarification! The current approach is cleaner than consciously stripping the typeOptions value, so let's keep it this way.

ui/src/router/components/form/components/ensembler_config/typeOptions.js

deadlycoconuts · 2022-09-23T03:41:30Z

ui/src/services/job/JobStatus.js

@@ -3,22 +3,22 @@ import { Enum, EnumValue } from "../enum/Enum";
 export const JobStatus = Enum({
  PENDING: EnumValue("pending", {
    label: "Pending",
-    color: "#FEC514",
+    color: "warning",


Changing these colours to use the default EUI colour template wherever possible.

krithika369 · 2022-09-23T04:12:52Z

ui/src/services/status/Status.js

    iconType: "cross",
  }),
  PENDING: EnumValue("pending", {
    label: "Updating",
-    color: "#FEC514",
+    color: "warning",


Thanks for changing these. What about the "Not Deployed" status? Shall we change it to default ? And any previous occurrence using subdued to default as well? https://elastic.github.io/eui/#/display/badge

Hmm default isn't as dark as #6A717D but I guess that works too :) I'll replace all the old subdued values (or the hexes corresponding to them) with default then :)

…engines

… exp engine ui

krithika369 · 2022-09-23T09:25:26Z

ui/src/components/remote_component/RemoteLoaderComponent.js

@@ -17,9 +17,10 @@ const LoadDynamicScript = ({ url, setReady, setFailed }) => {
 };

 // Dynamic Script Loading component wrapper
-export const ExperimentEngineLoaderComponent = ({
+export const RemoteLoaderComponent = ({


Nit: I think it's ok to keep this component as it was, except may be make the componentName configurable. After all, the std ensembler UI is tied to the experiment engine and it's the engine whose configs we are loading.

If it helps, we can rename it from ExperimentEngineLoaderComponent -> ExperimentEngineComponentLoader :D

Wait so do you mean we should keep both components separate, i.e. StandardEnsemblerLoaderComponent and ExperimentEngineLoaderComponent, or combining them as @terryyylim had suggested? 😅

I just noticed the older comment. I do think it makes sense to combine the way you have it. But I don't think this should be called a generic RemoteLoaderComponent because the logic inside is still looking at getting some url, configs (which are coupled to the experiment engine). So, the implementation can remain as is, but I think it still makes sense to call it something related to experiment engine - hence my suggestion is to use ExperimentEngineComponentLoader as the name.

Ok! I've just renamed the component as you have suggested!

terryyylim

Thanks @deadlycoconuts , I'll leave @krithika369 to approve this PR since there's the rename comment yet to be resolved.

terryyylim · 2022-09-26T03:52:02Z

ui/src/router/components/form/components/ensembler_config/typeOptions.js

@@ -14,13 +14,26 @@ const typeOptions = [
  {
    value: "standard",
    inputDisplay: "Standard",
+    expenginetype: "standard",


I see, thanks for clarification! The current approach is cleaner than consciously stripping the typeOptions value, so let's keep it this way.

deadlycoconuts · 2022-09-26T08:15:48Z

Thanks a lot for all the comments everyone! I've added some short descriptions to the docs about standard ensemblers with custom experiment engines. Besides that there shouldn't be other changes besides those that are meant to address the comments above.

krithika369

The docs LGTM. Thanks for this!

krithika369 · 2022-09-26T09:34:56Z

docs/how-to/create-a-router/configure-ensembler.md

+configuration does not correspond to any of the routes configured, or when the user-configured path is invalid with 
+respect to the treatment configuration received.
+
+The UI for Standard Ensemblers with Custom Experiment Engines is consumed directly as a remote component that is 


This line is better removed from the user docs - is it more of a dev doc detail (thanks for updating it).

deadlycoconuts · 2022-09-27T01:48:36Z

Thanks a lot for the comments everyone! I'll be merging this right away! 🚀🚀

deadlycoconuts self-assigned this Sep 8, 2022

deadlycoconuts changed the title ~~Allow standard ensemblers with custom experiment engines~~ Make Turing API inject route_name_path into Turing Router Sep 8, 2022

deadlycoconuts changed the title ~~Make Turing API inject route_name_path into Turing Router~~ Configure Turing API to use Standard Ensembler for Custom Experiment Engines Sep 8, 2022

deadlycoconuts closed this Sep 9, 2022

deadlycoconuts deleted the allow_standard_ensemblers_with_custom_experiment_engines branch September 9, 2022 06:54

deadlycoconuts restored the allow_standard_ensemblers_with_custom_experiment_engines branch September 9, 2022 06:55

deadlycoconuts deleted the allow_standard_ensemblers_with_custom_experiment_engines branch September 9, 2022 06:59

deadlycoconuts restored the allow_standard_ensemblers_with_custom_experiment_engines branch September 9, 2022 06:59

deadlycoconuts reopened this Sep 9, 2022

deadlycoconuts changed the title ~~Configure Turing API to use Standard Ensembler for Custom Experiment Engines~~ Configure Turing UI to use Standard Ensembler with Custom Experiment Engines Sep 9, 2022

deadlycoconuts changed the title ~~Configure Turing UI to use Standard Ensembler with Custom Experiment Engines~~ Apply Standard Ensembler Routing Policy with Custom Experiment Engines Sep 12, 2022

deadlycoconuts changed the title ~~Apply Standard Ensembler Routing Policy with Custom Experiment Engines~~ Configure Turing API to use Standard Ensembler for Custom Experiment Engines Sep 12, 2022

deadlycoconuts force-pushed the allow_standard_ensemblers_with_custom_experiment_engines branch from 5c96eda to 7273db7 Compare September 12, 2022 03:55

deadlycoconuts mentioned this pull request Sep 12, 2022

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

Merged

deadlycoconuts changed the title ~~Configure Turing API to use Standard Ensembler for Custom Experiment Engines~~ Configure Turing UI to use Standard Ensembler for Custom Experiment Engines Sep 12, 2022

deadlycoconuts commented Sep 12, 2022

View reviewed changes

deadlycoconuts force-pushed the allow_standard_ensemblers_with_custom_experiment_engines branch 5 times, most recently from f5e889e to 93a75ba Compare September 19, 2022 03:01

deadlycoconuts force-pushed the allow_standard_ensemblers_with_custom_experiment_engines branch 2 times, most recently from e6dad79 to 2fd624a Compare September 21, 2022 02:49

deadlycoconuts marked this pull request as ready for review September 21, 2022 04:06

deadlycoconuts commented Sep 21, 2022

View reviewed changes

deadlycoconuts requested a review from a team September 21, 2022 05:35

deadlycoconuts mentioned this pull request Sep 21, 2022

Add standard ensembler UI caraml-dev/xp#37

Merged

terryyylim requested changes Sep 22, 2022

View reviewed changes

deadlycoconuts commented Sep 23, 2022

View reviewed changes

deadlycoconuts force-pushed the allow_standard_ensemblers_with_custom_experiment_engines branch from d1e93e0 to d56b00d Compare September 23, 2022 04:02

krithika369 reviewed Sep 23, 2022

View reviewed changes

deadlycoconuts added 12 commits September 23, 2022 12:23

Update type ensembler options to allow std ensemblers for custom exp …

87f8e1a

…engines

Make create and edit router consume standard ensembler component from…

cdaceec

… exp engine ui

Add route name path to standard ensembler config struct

2ba195b

Clean up props to be passed to remote ensembler component

65a184c

Add routes prop to pass to the remote standard ensembler

f42ba35

Update typeOptions to return correct variant of standard ensembler

6b1416e

Add standard ensembler config view

d151b10

Rename prop for StandardEnsemblerLoaderComponent

f26e615

Make status colours use default EUI palette as much as possible

04e5faa

Remove redundant empty tags

5a3a938

Refactor components to use common RemoteLoaderComponent

620adc1

Replace colour hexes corresponding to subdued to default

95faa74

deadlycoconuts force-pushed the allow_standard_ensemblers_with_custom_experiment_engines branch from d56b00d to 95faa74 Compare September 23, 2022 04:23

krithika369 reviewed Sep 23, 2022

View reviewed changes

terryyylim reviewed Sep 26, 2022

View reviewed changes

deadlycoconuts added 3 commits September 26, 2022 12:57

Add docs for standard ensembler

5f1cf84

Remove redundant description for std ensembler and custom exp engines

4478b71

Rename generic component loader as ExperimentEngineComponentLoader

572fe8d

deadlycoconuts requested a review from terryyylim September 26, 2022 08:15

krithika369 approved these changes Sep 26, 2022

View reviewed changes

Remove redundant line from docs

d5ad99b

terryyylim approved these changes Sep 27, 2022

View reviewed changes

deadlycoconuts merged commit 296251d into caraml-dev:main Sep 27, 2022

deadlycoconuts deleted the allow_standard_ensemblers_with_custom_experiment_engines branch September 27, 2022 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

deadlycoconuts commented Sep 8, 2022 •

edited

Loading

deadlycoconuts Sep 12, 2022

deadlycoconuts Sep 21, 2022 •

edited

Loading

deadlycoconuts Sep 21, 2022 •

edited

Loading

deadlycoconuts Sep 21, 2022

deadlycoconuts Sep 21, 2022

krithika369 Sep 23, 2022

krithika369 Sep 23, 2022

deadlycoconuts Sep 25, 2022

krithika369 Sep 26, 2022

deadlycoconuts Sep 26, 2022

terryyylim left a comment

terryyylim Sep 22, 2022

deadlycoconuts Sep 23, 2022 •

edited

Loading

terryyylim Sep 22, 2022

deadlycoconuts Sep 23, 2022

terryyylim Sep 22, 2022

deadlycoconuts Sep 23, 2022

terryyylim Sep 26, 2022

deadlycoconuts Sep 23, 2022

krithika369 Sep 23, 2022

deadlycoconuts Sep 23, 2022 •

edited

Loading

krithika369 Sep 23, 2022

deadlycoconuts Sep 25, 2022

krithika369 Sep 26, 2022

deadlycoconuts Sep 26, 2022

terryyylim left a comment

terryyylim Sep 26, 2022

deadlycoconuts commented Sep 26, 2022

krithika369 left a comment

krithika369 Sep 26, 2022 •

edited

Loading

deadlycoconuts commented Sep 27, 2022

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

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

Conversation

deadlycoconuts commented Sep 8, 2022 • edited Loading

Context

Main Modifications

Choose a reason for hiding this comment

deadlycoconuts Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

deadlycoconuts Sep 21, 2022 • edited Loading

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

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

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts Sep 23, 2022 • edited Loading

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

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

deadlycoconuts Sep 23, 2022 • edited Loading

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

Choose a reason for hiding this comment

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts commented Sep 26, 2022

krithika369 left a comment

Choose a reason for hiding this comment

krithika369 Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

deadlycoconuts commented Sep 27, 2022

deadlycoconuts commented Sep 8, 2022 •

edited

Loading

deadlycoconuts Sep 21, 2022 •

edited

Loading

deadlycoconuts Sep 21, 2022 •

edited

Loading

deadlycoconuts Sep 23, 2022 •

edited

Loading

deadlycoconuts Sep 23, 2022 •

edited

Loading

krithika369 Sep 26, 2022 •

edited

Loading