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

Update workflows to use Python 3.7 for ensembler engines and sdk #190

Merged
merged 4 commits into from
Mar 30, 2022

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Mar 29, 2022

Context

With the PR #188, the Python versions for the Turing SDK as well as both ensembler engines have been fixed at 3.7.*. While these changes have been made on these components, the workflows for these components have not been updated yet.

This PR serves to update these workflows to utilise Python 3.7 accordingly.

Additional Fixes

  • Tiny fix to the Turing SDK to handle cases whereby RouterConfig is not returned as part of a response body containing Routers
  • Tiny fix to the Turing UI to allow for text wrapping when the docker image name is too long

@deadlycoconuts deadlycoconuts self-assigned this Mar 29, 2022
@deadlycoconuts deadlycoconuts requested a review from a team March 29, 2022 02:02
@deadlycoconuts deadlycoconuts marked this pull request as ready for review March 29, 2022 02:06
if self._config is not None:
return RouterConfig(name=self.name, environment_name=self.environment_name, **self._config)
else:
return None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this if-else check to handle cases whereby querying the API for a certain router returns an object without a RouterConfig. This occurs notably when a new Turing Router is pending (being deployed).

@@ -11,7 +11,7 @@
.euiPanel--configSection {
.euiDescriptionList.euiDescriptionList--responsiveColumn {
> * {
@include euiTextTruncate;
@include euiTextBreakWord;
Copy link
Contributor Author

@deadlycoconuts deadlycoconuts Mar 30, 2022

Choose a reason for hiding this comment

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

@krithika369, I can't seem to override this mixin that truncates quite a number of text descriptions in other UI components - I tried creating custom style rules for the Container table but they don't seem to be able override this mixin either :/ Would it be too drastic if I were to swap this mixin out? I'm not sure why the decision was made to truncate long text in other config display panels though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh we shouldn't change it globally for all rows.

I tried creating custom style rules for the Container table

Could you share the example here? What was tried and where it was placed?

Copy link
Contributor Author

@deadlycoconuts deadlycoconuts Mar 30, 2022

Choose a reason for hiding this comment

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

So I tried creating this custom style class and using it for the EuiDecriptionList of a ContainerConfigTable, but it doesn't overwrite any of the truncation styles:

.euiDescriptionList--responsiveColumn.containerConfig {
  .euiDescriptionList__description {
    @include euiTextBreakWord;
  }
}

Explicitly specifying the style of the euiTextBreakWord mixin as found here doesn't seem to work either:

.euiDescriptionList--responsiveColumn.containerConfig {
  .euiDescriptionList__description {
    overflow-wrap: break-word !important;
    word-wrap: break-word !important;
    word-break: break-word;
  }
}

Copy link
Contributor Author

@deadlycoconuts deadlycoconuts Mar 30, 2022

Choose a reason for hiding this comment

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

Another thing I've tried to try was to add <div className="eui-textBreakWord"> tags to the description of the item corresponding to the image name within the items list of the ContainerConfigTable as:

{
  title: "Image",
  description: (
      <div className="eui-textBreakWord">
        thisisaverylargexcepetionnamewithalotofwordsthatatsomepointaregoingtobetruncated
      </div>
    )
}

Although this doesn't 'truncate' the line and replace the missing characters with an ellipsis, it doesn't break the line either... generating something like this instead:
Screenshot 2022-03-30 at 10 59 51 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see.. I was going to suggest something along the lines of your second example to override the properties for the single row's description. But adding the div is probably messing with some other flex properties. I will try to have a stab at it to understand better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot! I'll see what I can try out from my end!

Copy link
Collaborator

@krithika369 krithika369 Mar 30, 2022

Choose a reason for hiding this comment

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

Using span in combination with inline style overrides seems to do it:

const items = [
    {
      title: "Image",
      description: (<span style={{
        wordBreak: "break-word",
        textOverflow: "unset",
        whiteSpace: "break-spaces"
      }} >
        {image}
      </span >),
    },

We're having to override some existing !important properties which doesn't seem easy with classes.
Screenshot 2022-03-30 at 1 28 45 PM

It may not be the most graceful solution but given that we want to relax the truncation behavior for the one description only, I think it may be acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow thanks for discovering that! I'll include your code snippet in!

@deadlycoconuts
Copy link
Contributor Author

Thanks everyone for your comments/approval! Merging this! 🚀

@deadlycoconuts deadlycoconuts merged commit 7caf52e into caraml-dev:main Mar 30, 2022
krithika369 added a commit that referenced this pull request Jun 6, 2022
* 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]>
krithika369 added a commit that referenced this pull request Jun 9, 2022
* 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]>
@deadlycoconuts deadlycoconuts deleted the fix_workflows branch August 5, 2022 08:31
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