-
Notifications
You must be signed in to change notification settings - Fork 23
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 SDK functionality #152
Add create router SDK functionality #152
Conversation
sdk/turing/experiment_config.py
Outdated
def config(self, config): | ||
if config is not None and 'project_id' in config: | ||
config['project_id'] = int(config['project_id']) | ||
self._config = config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This project_id
check is a little awkward; it's there because when project_id
gets returned as a response within the config
attribute, its value gets parsed as a float
by the autogenerated API classes, which when sent back as part of a request, receives a 400 Bad Request message since the API expects an int
for the project_id
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extensive test suites @deadlycoconuts , left a few comments for your consideration!
@@ -29,6 +29,7 @@ components: | |||
properties: | |||
name: | |||
type: "string" | |||
pattern: '^[a-zA-Z0-9_]*$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex expressions look slightly different from those of the UI as the case insensitive flag i
doesn't seem to work with OpenAPI generator. As a workaround, I enumerated the different letter cases (i.e. added A-Z
) in each of the expressions where the case insensitive flag was used.
aa56cfc
to
d3b7b17
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks @deadlycoconuts. Could you please also include samples on how to use this SDK under https://github.com/gojek/turing/tree/main/sdk/samples?
Alright sure, I'll add them in in the next PR :) |
…ype and config attribute
…fig into dataclasses
d3b7b17
to
afa1d0e
Compare
Context:
Currently the Turing SDK only supports batch experiments by covering only operations related to ensemblers and ensembling jobs, and we are now expanding its scope to cover online experiments by extending the SDK to cover operations involving other Turing components. Previously we've already implemented the SDK to supporting the listing of routers in PR #148, and this PR includes additional functionalities involving the setting up of routers.
Features:
This PR involves implementing a functionality corresponding to one of the API endpoints involving routers, i.e. creating a new router given a router configuration.
Example:
Example:
Modifications
sdk/turing/router/*
- new Python files containing the new SDK classes for router componentssdk/turing/router/router.py
- addition of a newRouter.create
method to create a new Turing router with a given router configurationsdk/turing/router/session.py
- addition of a newcreate_router
method used byRouter.create
to call the API method associated with creating a new Turing routerapi/api/specs/*
- addition of regex expressions that will be enforced by API classes generated by OpenAPI Generator; acts as a central source of truth for validating user inputsdk/turing/generated/model/*
- autogenerated API classes changed by the OpenAPI generator given the changes made to the OpenAPI specs