Add standard ensembler UI #37

deadlycoconuts · 2022-09-19T17:49:01Z

What this PR does / why we need it:
With the changes introduced in Turing to support the usage of Standard Ensemblers with Custom Experiment Engines, the Turing Experiments UI has been updated in this PR to expose Standard Ensembler components consumed by the Turing UI.

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

StandardEnsemblerConfigDetails

EditStandardEnsemblerConfig

Sample User Workflow

Screen.Recording.2022-09-21.at.5.46.07.PM.mov

In addition, the appConfig now has a new routeNamePathPrefix field that specifies the path prefix that gets appended to a user-defined treatment configuration. This path prefix reflects the nesting of the treatment configuration within the final response payload that the treatment service, and/or subsequently the Turing Experiments plugin, sends back to the client.

For example, if the user-defined treatment configuration is:

{
    "route_name": "control"
    ...
}

and the client response is as follows:

{
    "treatment": {
        "configuration": {
            "route_name": "control"
            ...
        }
        ....
    }
    ...
}

then the path prefix will have to be specified as treatment.configuration. (including the final period).

Main Modifications

ui/craco.config.js - Addition of the exposed component paths to be consumed by the ModuleFederationPlugin
ui/src/config.js - Addition of the routeNamePathPrefix field to the appConfig
ui/src/turing/components/configuration/StandardEnsemblerConfigDetails.js - Addition of the new StandardEnsemblerConfigDetails component to be exposed
ui/src/turing/components/form/EditStandardEnsemblerConfig.js - Addition of the new EditStandardEnsemblerConfig

…s and routes

terryyylim

LGTM on the whole, left some comments to be addressed, thank you @deadlycoconuts !

ui/src/turing/components/configuration/StandardEnsemblerConfigDetails.js

ui/src/turing/components/configuration/standard_ensembler_config/RouteNamePathConfigGroup.js

ui/src/turing/components/form/standard_ensembler/LinkedExperimentsContextMenu.js

ui/src/turing/components/form/standard_ensembler/RouteNamePathFlyout.js

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js

terryyylim · 2022-09-22T06:58:36Z

ui/src/turing/components/form/standard_ensembler/LinkedExperimentsContextMenu.js

+    </EuiButtonEmpty>
+  );
+
+  return isButtonPopoverOpen[item.id] ? (


Let's use && and not have a ternary that returns null here.

Thanks for catching that! I've just edited it!

Correction: I remembered I need null as a return value because a function passed to the render prop needs to return minimally a null response as opposed to undefined which is what may happen if you use && and the first condition returns undefined

Should be addressed by this suggestion - #37 (comment).

terryyylim · 2022-09-22T07:08:15Z

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js

+      field: "status",
+      width: "5px",
+      render: (_, item) => {
+        const isAssigned = routeToExperimentMappings[item.id] ?


status and route_name column computations seem to be duplicated here. Would it make sense to pre-aggregate all column values prior to passing them into the items prop of EuiInMemoryTable?

All the columns (with the exception of status) actually use multiple column values (of the rows in items, which represents the routes configured) to render the cells of the table. While it's easy to precompute the cells for status (isAssigned is the only value needed to determine status) and store them as a column, route_name for example needs both the value of isAssigned as well as the id of each route.

This poses a problem because it's not possible to pass 2 (or more) values into the field prop of EuiTableFieldDataColumnType for the EuiBasicTable to extract both values and then pass them as arguments to the function specified in the render prop. This means that whatever precomputed values aggregated in advance would only be useful for the partially rendering each cell, because the function of the render prop would still have to access or compute other values from each row on-the-fly to determine how the cell ultimately gets rendered.

That said, I think this makes aggregating the computations in advance less useful than expected, since we aren't just extracting a single value to display for each cell, and still relying on the render prop to perform the rendering.

On the other hand, I've not tried precomputing each column as a ReactNode component and then getting the table to extract these values as is (without having to rely on the render function) i.e. having the field-s set as "column_1", "column_2", "column_3" ..., and then having these keys within each of the elements in items corresponding to <Column1... />, <Column2... />, <Column3... />, ... 🤔

Just a slight update on this, I think we can just use id in the field for all of the columns, and then get the render function to do something with the id since we're almost always using the routeToExperimentMappings with the route name.

deadlycoconuts · 2022-09-23T04:14:28Z

ui/src/services/experiment/ExperimentStatus.js

@@ -13,7 +13,7 @@ export const getExperimentStatus = (experiment) => {
  const statusMapping = {
    Deactivated: {
      label: "Deactivated",
-      color: "subdued",
+      color: "#6A717D",


Fixed this colour which is no longer supported as part of the EUI colour palette; it now needs to be specified manually with a colour hex.

krithika369 · 2022-09-23T05:52:14Z

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js

+}) => {
+  const { appConfig } = useConfig();
+
+  const [isButtonPopoverOpen, setIsButtonPopoverOpen] = useState(routes.reduce((m, r) => {m[r.id] = {running: false, scheduled: false}; return m}, {}));


Hmmm having this state in the parent component makes the implementation a little more complex and less coupled with the button that it belongs to. Can this be defined directly inside LinkedExperimentsContextMenu component so that each clickable item has its own state? And can it simply track true/false ? Then, if any of them are true, we can use the routeToExperimentMappings to list the experiments in the popover, for that specific status - right?

We will be creating more state variables but since they are all lean, I don't think that should be a problem.

I remember making a conscious choice to manage all the states within the parent because creating the individual states within LinkedExperimentsContextMenu somehow threw some error or minimally a warning regarding the creation of individual states by looping through with the function in render. I'll try to reproduce the issue again and share that error again.

Okay I can't seem to get the error to reappear, maybe it was something else :/ I've made the changes to track the state of each button within each LinkedExperimentsContextMenu, and we'll be using useToggle to get the true/false state of the button.

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js

…r rows

deadlycoconuts · 2022-09-23T10:06:23Z

Thanks everyone for all the comments! I should've most of them covered. There's some significant refactoring done to LinkedRouteTable and LinkedExperimentsContextMenu, so it'd be nice to take a look at that!

terryyylim

Thanks @deadlycoconuts , PR looks cleaner now! I left some small comments - feel free to merge after addressing them 😄

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js

ui/src/turing/components/form/standard_ensembler/LinkedExperimentsContextMenu.js

ui/src/turing/components/form/standard_ensembler/RouteNamePathFlyout.js

krithika369 · 2022-09-26T06:24:40Z

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js

+
+  // this stringified value of routes below allows the React effect below to mimic a deep comparison when changes to the
+  // array routes are made
+  const stringifiedRoutes = JSON.stringify(routes)


Would it be better to use the ids only?

const stringifiedRoutes = routes.map(e => e.id).join()

deadlycoconuts · 2022-09-26T07:53:27Z

It seems like all of the issues on this PR has been resolved; I'll be merging this in a short while! Thanks a lot everyone for your time! :)

deadlycoconuts self-assigned this Sep 19, 2022

deadlycoconuts force-pushed the add_standard_ensembler_ui branch from 6a7b396 to 24cad60 Compare September 20, 2022 11:41

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

deadlycoconuts requested a review from a team September 21, 2022 09:51

deadlycoconuts added 14 commits September 22, 2022 13:22

Add standard ensembler component

5feb2ac

Simplify react effects used in affected routes list panel

b8cbf82

Add popover to list the affected experiments

d0b8b0a

Fix empty titles in EuiDescriptionList

2f3ea20

Add context menu to display affected experiments

b8365fb

Fix styles in prop passing

bb4ace6

Add tooltip and color to affected experiments menu

a7de94a

Add standard ensembler config view

7429a77

Fix lint comments

4289b85

Add help flyout to explain route name path prefix

9e56ccb

Update deprecated subdued EUI colour with colour hex

2762412

Rename affected experiments and routes component to linked experiment…

6e5cddf

…s and routes

Reword route path prefix explanation

9dd11c7

Fix lint comments

968fdde

deadlycoconuts force-pushed the add_standard_ensembler_ui branch from e8b3055 to 968fdde Compare September 22, 2022 05:23

terryyylim suggested changes Sep 22, 2022

View reviewed changes

terryyylim reviewed Sep 22, 2022

View reviewed changes

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js Outdated Show resolved Hide resolved

terryyylim reviewed Sep 22, 2022

View reviewed changes

deadlycoconuts added 4 commits September 23, 2022 11:02

Fix incorrect language type specified for code block

0017478

Replace style used to pass items to the list in RouteNamePathConfigGroup

23ad1a4

Rename misleading variable names and remove redundant tags

0dd528c

Replace relative import path with absolute path

a71f04e

deadlycoconuts commented Sep 23, 2022

View reviewed changes

Replace colour hexes corresponding to subdued to default

7dc089e

krithika369 reviewed Sep 23, 2022

View reviewed changes

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js Outdated Show resolved Hide resolved

krithika369 reviewed Sep 23, 2022

View reviewed changes

ui/src/turing/components/form/standard_ensembler/LinkedRoutesTable.js Outdated Show resolved Hide resolved

deadlycoconuts added 5 commits September 23, 2022 16:48

Refactor experiments loading into a context provider

73d673f

Standardise naming of all providers to be singular wherever logical

6234f6d

Make popover management distributed across each component

caa20c7

Refactor LinkedRoutesTable to utilise id from list of routes to rende…

7005210

…r rows

Revert incorrect edit made to docstrings

584c0e3

deadlycoconuts requested a review from terryyylim September 23, 2022 10:06

terryyylim approved these changes Sep 26, 2022

View reviewed changes

deadlycoconuts added 2 commits September 26, 2022 13:53

Refactor LinkedRoutesTable

fd79dbf

Consume route name path in flyout from config

6f82e3e

krithika369 reviewed Sep 26, 2022

View reviewed changes

Simplify effect dependency on routes in LinkedRoutesTable

5f8d17f

deadlycoconuts merged commit 734b0ab into caraml-dev:main Sep 26, 2022

deadlycoconuts deleted the add_standard_ensembler_ui branch September 26, 2022 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add standard ensembler UI #37

Add standard ensembler UI #37

deadlycoconuts commented Sep 19, 2022 •

edited

Loading

terryyylim left a comment

terryyylim Sep 22, 2022

deadlycoconuts Sep 22, 2022

deadlycoconuts Sep 23, 2022 •

edited

Loading

terryyylim Sep 26, 2022

terryyylim Sep 22, 2022

deadlycoconuts Sep 23, 2022

deadlycoconuts Sep 23, 2022

deadlycoconuts Sep 23, 2022

krithika369 Sep 23, 2022

deadlycoconuts Sep 23, 2022 •

edited

Loading

deadlycoconuts Sep 23, 2022

deadlycoconuts commented Sep 23, 2022

terryyylim left a comment

krithika369 Sep 26, 2022

deadlycoconuts commented Sep 26, 2022

Add standard ensembler UI #37

Add standard ensembler UI #37

Conversation

deadlycoconuts commented Sep 19, 2022 • edited Loading

Main Modifications

terryyylim left a 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

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

deadlycoconuts commented Sep 23, 2022

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadlycoconuts commented Sep 26, 2022

deadlycoconuts commented Sep 19, 2022 •

edited

Loading

deadlycoconuts Sep 23, 2022 •

edited

Loading

deadlycoconuts Sep 23, 2022 •

edited

Loading