Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add create router version feature #179

Merged

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Mar 15, 2022

Context

When updating a router with a new configuration, Turing API is currently only able to create a new router version and then deploy it automatically as a single atomic step. This may not be ideal for users who simply intend to create new versions without deploying them, since there isn't an endpoint that support such an operation.

This PR thus introduces a new API endpoint method for Turing API to create a new router version without deploying it automatically, and includes updates to Turing UI and Turing SDK to support this new functionality.

Features

  • New endpoint to support the creation of new router versions without deploying them automatically (POST operation with the existing /projects/{project_id}/routers/{router_id}/versions endpoint)

  • Introduction of new buttons at the 'Edit Router` diff and confirmation page to reflect the new option to create new router versions without deploying them
    Screenshot 2022-03-18 at 4 59 25 PM
    Screenshot 2022-03-18 at 5 01 36 PM

  • Introduction of a new SDK method to create new router versions without deploying them

new_router_config = RouterConfig(
    routes=routes,
    rules=rules,
    default_route_id="test",
    experiment_engine=experiment_config,
    resource_request=resource_request,
    timeout="100ms",
    log_config=log_config,
    enricher=None,
    ensembler=None
)

turing.router.config.router_version.RouterVersion.create(new_router_config, my_router_id)

Main Modifications

  • api/turing/api/router_versions_api.go - Addition of a new request handler for the new API endpoint method
  • api/api/* - Changes to the OpenAPI specs from the addition of the new API endpoint method
  • ui/src/router/edit/components/VersionComparisonView.js - Addition of new buttons and onclick handlers to the diff page
  • ui/src/router/edit/EditRouterView.js - Addition of additional effects and API calls upon submission
  • ui/src/router/components/form/components/VersionCreationSummary.js - Creation of a new confirmation page for router version creation
  • sdk/turing/router/router.py - Addition of a method to the Router SDK class to support router version creation
  • sdk/turing/session.py - Addition of a support method to support router version creation
  • sdk/turing/generated/api/router_api.py - Changes to OpenAPI autogenerated classes from addition of the new API endpoint method

Tiny Fixes

  • sdk/requirements.txt - Pinning of cloudpickle==1.2.2 to the requirements list to ensure compatibility with the Mlflow dependency of the pyfunc ensembler service engine

@deadlycoconuts deadlycoconuts requested a review from a team March 15, 2022 03:16
@deadlycoconuts deadlycoconuts self-assigned this Mar 15, 2022
@deadlycoconuts deadlycoconuts force-pushed the add_create_router_version_feature branch from c6442d2 to 3260576 Compare March 15, 2022 03:17
@deadlycoconuts deadlycoconuts marked this pull request as ready for review March 15, 2022 03:23
@romanwozniak
Copy link
Contributor

Minor feedback: can we use the secondary color from the elastic EUI color schema (it was removed in the newer version of EUI, but it can be replaced with a similar color) for the Save button, instead of this mustard-yellow color?

Also, what do you all think about simplifying the button labels to Save and Deploy (from the proposed Create without Deploying and Create and Deploy) and instead, add the explanation about the action consequences into the confirmation pop-up window?

@deadlycoconuts
Copy link
Contributor Author

Minor feedback: can we use the secondary color from the elastic EUI color schema (it was removed in the newer version of EUI, but it can be replaced with a similar color) for the Save button, instead of this mustard-yellow color?

Ahah, I thought yellow looked interesting 😅 The secondary colour is actually green, which also happens to be the same colour as the 'Deployed' symbol, which may be a little confusing:
Screenshot 2022-03-15 at 11 44 35 AM
vs
Screenshot 2022-03-15 at 11 52 28 AM

There's also text which doesn't look too bad:
Screenshot 2022-03-15 at 11 44 22 AM

Otherwise success (successor to the secondary colour), accent, ghost all have a similar shade to the button below, which I think fades a little too much into the background:
Screenshot 2022-03-15 at 11 51 26 AM

Also, what do you all think about simplifying the button labels to Save and Deploy (from the proposed Create without Deploying and Create and Deploy) and instead, add the explanation about the action consequences into the confirmation pop-up window?
I think that sounds like a good idea, it frees up space and makes the UI look more minimalist.

@romanwozniak
Copy link
Contributor

romanwozniak commented Mar 15, 2022

You can also consider somethinbg like this:
158302733-7a85774c-e5c7-485b-b462-1444f7c926dc

Or, use the different shades of blue for Save and Deploy buttons:
Screenshot 2022-03-15 at 12 03 53 PM

@krithika369
Copy link
Collaborator

Ahah, I thought yellow looked interesting 😅 The secondary colour is actually green, which also happens to be the same colour as the 'Deployed' symbol, which may be a little confusing:

I agree with this. But the gray color alternative is also the same as "undeployed". :)

Of all the options being discussed, my personal preference is different shades of blue for Save vs Deploy:
Screenshot 2022-03-15 at 12 05 24 PM

@deadlycoconuts
Copy link
Contributor Author

Alright, lemme get those colours up :D

@krithika369
Copy link
Collaborator

Also, what do you all think about simplifying the button labels to Save and Deploy (from the proposed Create without Deploying and Create and Deploy) and instead, add the explanation about the action consequences into the confirmation pop-up window?

Sounds great! The original specs say "Update", "Update and Deploy". I think "Save" and "Deploy" is concise and still conveys the intention.

@deadlycoconuts I haven't reviewed the MR yet but I'd like to confirm - this change is only for the "Edit Router" workflow, right? (The "Create" on the buttons got me wondering.)

@deadlycoconuts
Copy link
Contributor Author

deadlycoconuts commented Mar 15, 2022

@deadlycoconuts I haven't reviewed the MR yet but I'd like to confirm - this change is only for the "Edit Router" workflow, right? (The "Create" on the buttons got me wondering.)

Ahah yes, I essentially just added an extra button in the existing "Edit Router" user workflow (to... fork the existing workflow? if that's the right way to say it?). And oh I used "Create" because they were supposed to hint at "Create (Version) and Deploy" and "Create (Version) without Deploying" 😅

@deadlycoconuts
Copy link
Contributor Author

deadlycoconuts commented Mar 15, 2022

Would this colour scheme be okay? It's one that uses the default colour palette for the buttons; there doesn't seem to be a way to easily get the lighter-shade blue (because it isn't in the default palette) and I think we'll have to manually pass CSS properties in order to do that:
Screenshot 2022-03-15 at 1 02 37 PM
^ Furthermore the 'Previous' button now corresponds with the 'Cancel' button from the previous page.

Edit: On second thought, we can just leave the customised values for this button without changing the palette (since this alternate colour isn't really used elsewhere):
Screenshot 2022-03-15 at 2 10 26 PM

@romanwozniak
Copy link
Contributor

Would this colour scheme be okay? It's one that uses the default colour palette for the buttons; there doesn't seem to be a way to easily get the lighter-shade blue (because it isn't in the default palette) and I think we'll have to manually pass CSS properties in order to do that: Screenshot 2022-03-15 at 1 02 37 PM ^ Furthermore the 'Previous' button now corresponds with the 'Cancel' button from the previous page.

Edit: On second thought, we can just leave the customised values for this button without changing the palette (since this alternate colour isn't really used elsewhere): Screenshot 2022-03-15 at 2 10 26 PM

I like the first option more, especially the part, that 'Previous' is EuiLinkButton

Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comprehensive PR and updating the SDK as well. As a side note, we also need to think about updating the user docs (which are currently duplicated in the internal repo for Confluence but we have public docs in this repo too) when the UI implementation changes.

api/api/specs/routers.yaml Outdated Show resolved Hide resolved
api/turing/api/router_versions_api.go Show resolved Hide resolved
sdk/tests/router/router_test.py Show resolved Hide resolved
ui/src/router/edit/EditRouterView.js Outdated Show resolved Hide resolved
@deadlycoconuts deadlycoconuts force-pushed the add_create_router_version_feature branch from 5ea359c to 87539f3 Compare March 17, 2022 06:51
Copy link
Contributor

@romanwozniak romanwozniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My motivation for using ?deploy=true|false flag was primarily defined by my understanding of the Edit Router call to be in fact:

  • Create New Router Version
  • (if deploy=true) Then Promote this New Version to serve Router's traffic

So, in this context adding this ?deploy flag makes sense (to me). However, as @krithika369 mentioned before, if you see this call as Edit router configuration with the new version, then this deploy flag is confusing.

In the longer term, it doesn't really matter because after the API controllers are refactored, then most of the business logic will be moved out from the controllers and there will be less of code duplication.

Currently these multiple conditional checks

if toDeploy {
  // ...
}

are not contributing to the code readability, so maybe we should separate these two operations altogether. So in addition to existing
PUT /proejects/{project_id}/routers/{router_id}
We can just add
POST /projects/{project_id}/routers/{router_id}/versions which will create a new version without promoting it.

wdyt @krithika369

P.S. @deadlycoconuts sorry for the miscommunication from my end

api/api/specs/routers.yaml Outdated Show resolved Hide resolved
api/api/specs/routers.yaml Outdated Show resolved Hide resolved
api/api/specs/routers.yaml Outdated Show resolved Hide resolved
api/turing/api/base_controller.go Outdated Show resolved Hide resolved
@deadlycoconuts deadlycoconuts force-pushed the add_create_router_version_feature branch from f0c25ce to 44375ba Compare March 21, 2022 03:46
path: "/projects/{project_id}/routers/{router_id}/versions",
body: request.CreateOrUpdateRouterRequest{},
handler: c.CreateRouterVersion,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled this back out from the recycle bin given the latest proposed changes.

@deadlycoconuts
Copy link
Contributor Author

Okay thanks everyone for the clarification! I've just reverted the changes made to this PR to make it align with the latest proposal to keep the 2 endpoints for the 2 functionalities separate:

  • PUT /projects/{project_id}/routers/{router_id}
  • POST /projects/{project_id}/routers/{router_id}/versions

Lemme know if there's anything you'd like to add, @romanwozniak and @krithika369

api/api/specs/routers.yaml Outdated Show resolved Hide resolved
api/api/specs/routers.yaml Outdated Show resolved Hide resolved
api/turing/api/router_versions_api.go Outdated Show resolved Hide resolved
api/turing/api/router_versions_api.go Outdated Show resolved Hide resolved
api/api/specs/routers.yaml Show resolved Hide resolved
sdk/turing/router/router.py Outdated Show resolved Hide resolved
ui/src/router/components/form/components/UpdateSummary.js Outdated Show resolved Hide resolved
ui/src/router/edit/EditRouterView.js Outdated Show resolved Hide resolved
ui/src/router/edit/EditRouterView.js Outdated Show resolved Hide resolved
sdk/samples/router/edit_a_router.py Show resolved Hide resolved
@deadlycoconuts deadlycoconuts force-pushed the add_create_router_version_feature branch from 93e3325 to 68f93c9 Compare March 22, 2022 09:13
@deadlycoconuts
Copy link
Contributor Author

deadlycoconuts commented Mar 22, 2022

Thanks @krithika369 for all the responses and suggestions. I've refactored the CreateOrUpdateRouterRequest to move BuildExperimentEngineConfig and BuildRouterVersion to the request.RouterConfig struct. There aren't many changes beyond that (except for the some refactoring of the unit tests).

I'll be merging this branch if nothing goes wrong with the CI pipeline! 🚀

@deadlycoconuts deadlycoconuts merged commit 2d9310e into caraml-dev:main Mar 22, 2022
@deadlycoconuts deadlycoconuts deleted the add_create_router_version_feature branch March 24, 2022 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants