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 env vars support to Pyfunc ensembler #181

Merged
Merged
Show file tree
Hide file tree
Changes from 44 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
57e74c4
Add create router version endpoint
deadlycoconuts Mar 11, 2022
08b02f4
Edit error message
deadlycoconuts Mar 11, 2022
714ef51
Update OpenAPI specs and API with new endpoint
deadlycoconuts Mar 11, 2022
6ec6fa1
Add tests for CreateRouterVersion
deadlycoconuts Mar 14, 2022
80e419a
Add UI components for creating router versions without deployment
deadlycoconuts Mar 14, 2022
470e139
Refactor UI submit event
deadlycoconuts Mar 14, 2022
872561c
Add SDK method to create new router version
deadlycoconuts Mar 15, 2022
6704b57
Add tests for create version SDK method
deadlycoconuts Mar 15, 2022
60df8b6
Refactor error handling
deadlycoconuts Mar 15, 2022
3814012
Rename buttons
deadlycoconuts Mar 15, 2022
7406595
Change button colours to shades of primary colour
deadlycoconuts Mar 15, 2022
a5d9ed4
Add custom CSS colours
deadlycoconuts Mar 15, 2022
30dc347
Refactor variables in router view
deadlycoconuts Mar 17, 2022
54ed9c1
Refactor confirmation messages
deadlycoconuts Mar 17, 2022
375e2c6
Update put operation to create router versions to a post operation
deadlycoconuts Mar 21, 2022
cd92d6d
Add SDK sample to reflect saving of router versions
deadlycoconuts Mar 21, 2022
1565b79
Remove redundant words from OpenAPI spec descriptions
deadlycoconuts Mar 21, 2022
ff62970
Rename UpdateSummary to RouterUpdateSummary for consistency
deadlycoconuts Mar 21, 2022
16fb219
Rename callback functions for clarity
deadlycoconuts Mar 21, 2022
ff808b9
Refactor VersionComparisonView to receive two onSubmit handlers
deadlycoconuts Mar 21, 2022
a30e976
Simplify edit a router sample
deadlycoconuts Mar 21, 2022
54a81f0
Refactor create_version to now be a classmethod of RouterVersion
deadlycoconuts Mar 21, 2022
807fcb7
Refactor CreateRouterVersion endpoint to use RouterVersionConfig
deadlycoconuts Mar 22, 2022
c09f578
Edit UI to use refactored schema for creating router version
deadlycoconuts Mar 22, 2022
0d93d2b
Refactor create_version to now be a classmethod of RouterVersion
deadlycoconuts Mar 22, 2022
3a9a6b9
Refactor create router version as router controller method
deadlycoconuts Mar 18, 2022
f53108b
Fix lint comments
deadlycoconuts Mar 18, 2022
be1788a
Reword comments in updateRouter
deadlycoconuts Mar 18, 2022
cd3d40b
Add comments in updateRouter
deadlycoconuts Mar 18, 2022
121b1a0
Add env vars to ensembler model and deployment service
deadlycoconuts Mar 15, 2022
cd1ede4
Add pyfunc env vars to UI display
deadlycoconuts Mar 15, 2022
7660d29
Fix env var config display
deadlycoconuts Mar 15, 2022
a521723
Add tests for env var in pyfunc_config
deadlycoconuts Mar 15, 2022
e75a2ea
Fix OpenAPI files
deadlycoconuts Mar 22, 2022
a3198d9
Fix rebasing bugs
deadlycoconuts Mar 22, 2022
1215fe3
Fix rebasing bugs
deadlycoconuts Mar 22, 2022
0ad9402
Remove leftover statements
deadlycoconuts Mar 23, 2022
5074882
Add migration scripts
deadlycoconuts Mar 23, 2022
4b1de57
Fix typo in error message
deadlycoconuts Mar 23, 2022
5707207
Fix typos in migration script comments
deadlycoconuts Mar 23, 2022
e1bea16
Fix typos in migration scripts
deadlycoconuts Mar 23, 2022
88299cf
Fix typos in migration scripts
deadlycoconuts Mar 23, 2022
3d98112
Standardise migration script naming
deadlycoconuts Mar 23, 2022
ac542b9
Fix typos in migration scripts
deadlycoconuts Mar 23, 2022
3da6679
Fix rebasing errors
deadlycoconuts Mar 23, 2022
d846112
Refactor pyfunc config view
deadlycoconuts Mar 23, 2022
e73687e
Remove redundant migration scripts
deadlycoconuts Mar 23, 2022
567ad35
Refactor router SDK class
deadlycoconuts Mar 23, 2022
cad27fa
Refactor pyfunc_config to prevent argument mutation
deadlycoconuts Mar 23, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions api/api/openapi.bundle.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1951,6 +1951,11 @@ components:
max_replica: 6
memory_request: memory_request
cpu_request: cpu_request
env:
- name: name
value: value
- name: name
value: value
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
Expand Down Expand Up @@ -2212,6 +2217,11 @@ components:
max_replica: 6
memory_request: memory_request
cpu_request: cpu_request
env:
- name: name
value: value
- name: name
value: value
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
Expand Down Expand Up @@ -2347,6 +2357,11 @@ components:
max_replica: 6
memory_request: memory_request
cpu_request: cpu_request
env:
- name: name
value: value
- name: name
value: value
timeout: timeout
nullable: true
properties:
Expand All @@ -2359,6 +2374,10 @@ components:
timeout:
pattern: ^[0-9]+(ms|s|m|h)$
type: string
env:
items:
$ref: '#/components/schemas/EnvVar'
type: array
required:
- ensembler_id
- project_id
Expand Down Expand Up @@ -2502,6 +2521,11 @@ components:
max_replica: 6
memory_request: memory_request
cpu_request: cpu_request
env:
- name: name
value: value
- name: name
value: value
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
Expand Down Expand Up @@ -2634,6 +2658,11 @@ components:
max_replica: 6
memory_request: memory_request
cpu_request: cpu_request
env:
- name: name
value: value
- name: name
value: value
timeout: timeout
updated_at: 2000-01-23T04:56:07.000+00:00
standard_config:
Expand Down
6 changes: 5 additions & 1 deletion api/api/specs/routers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ info:
.timeout: &timeout
type: "string"
pattern: '^[0-9]+(ms|s|m|h)$'

paths:
"/projects/{project_id}/routers":
get:
Expand Down Expand Up @@ -851,6 +851,10 @@ components:
$ref: "#/components/schemas/ResourceRequest"
timeout:
<<: *timeout
env:
type: "array"
items:
$ref: "common.yaml#/components/schemas/EnvVar"

ResourceRequest:
type: "object"
Expand Down
2 changes: 1 addition & 1 deletion api/db-migrations/000010_add_pyfunc_config.down.sql
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
-- Removes py_func_ref_config from table.
-- Removes pyfunc_config from table.
-- Hence, this migration involves data loss for ensemblers with type that is "pyfunc"
ALTER TABLE ensembler_configs DROP COLUMN pyfunc_config;

2 changes: 1 addition & 1 deletion api/db-migrations/000010_add_pyfunc_config.up.sql
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
-- Adds py_func_ref_config to table.
-- Adds pyfunc_config to table.
ALTER TABLE ensembler_configs ADD pyfunc_config jsonb;

9 changes: 9 additions & 0 deletions api/db-migrations/000011_add_pyfunc_config_env.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-- Migrate pyfunc_config data from new schema to old schema (remove the env column)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tip: this can be simplified using the - operator like: https://stackoverflow.com/a/35283553

UPDATE ensembler_configs
SET pyfunc_config = (SELECT json_build_object(
'timeout', pyfunc_config ->> 'timeout',
'project_id', (pyfunc_config ->> 'project_id')::int,
'ensembler_id', (pyfunc_config ->> 'ensembler_id')::int,
'resource_request', (pyfunc_config ->> 'resource_request')::jsonb
))
WHERE type ='pyfunc';
10 changes: 10 additions & 0 deletions api/db-migrations/000011_add_pyfunc_config_env.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
-- Migrate pyfunc_config data from old schema to new schema (with new 'env' field set as an empty array by default)
UPDATE ensembler_configs
SET pyfunc_config = (SELECT json_build_object(
'env', '[]'::jsonb,
Copy link
Contributor Author

@deadlycoconuts deadlycoconuts Mar 23, 2022

Choose a reason for hiding this comment

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

Not sure if this is really necessary since the only pyfunc ensemblers that have been created without this field are the ones in dev, but for consistency's sake I thought having a migration script for this would make things neater. Alternatively, we could consider implementing some lazy migration of the saved pyfunc ensemblers in the API?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I feel it's better to not have this in the migration, just to keep the migration history lean.

This feature is still under development. :) We don't really need to worry since we haven't cut any release with migration file # 10 (and as a consequence, we also haven't released this feature to our internal users, as you've noted). We can run this data migration manually on the internal dev DB.

'timeout', pyfunc_config ->> 'timeout',
'project_id', (pyfunc_config ->> 'project_id')::int,
'ensembler_id', (pyfunc_config ->> 'ensembler_id')::int,
'resource_request', (pyfunc_config ->> 'resource_request')::jsonb
))
WHERE type ='pyfunc';
2 changes: 1 addition & 1 deletion api/turing/api/request/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (r RouterConfig) BuildRouterVersion(
}
if r.Ensembler.Type == models.EnsemblerPyFuncType {
if r.Ensembler.PyfuncConfig == nil {
return nil, errors.New("missing ensembler pyfunc reference config")
return nil, errors.New("missing ensembler pyfunc config")
}

// Verify if the ensembler given by its ProjectID and EnsemblerID exist
Expand Down
2 changes: 2 additions & 0 deletions api/turing/models/ensembler.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ type EnsemblerPyfuncConfig struct {
ResourceRequest *ResourceRequest `json:"resource_request" validate:"required"`
// Request timeout in duration format e.g. 60s
Timeout string `json:"timeout" validate:"required"`
// Environment variables to set in the container
Env EnvVars `json:"env" validate:"required"`
}

type ExperimentMapping struct {
Expand Down
2 changes: 1 addition & 1 deletion api/turing/service/router_deployment_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ func (ds *deploymentService) buildEnsemblerServiceImage(
Timeout: routerVersion.Ensembler.PyfuncConfig.Timeout,
Endpoint: PyFuncEnsemblerServiceEndpoint,
Port: PyFuncEnsemblerServicePort,
Env: models.EnvVars{},
Env: routerVersion.Ensembler.PyfuncConfig.Env,
ServiceAccount: "",
}

Expand Down
7 changes: 2 additions & 5 deletions sdk/samples/router/edit_a_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,14 @@ def main(turing_api: str, project: str):
new_router_config_to_save = router_1.config
new_router_config_to_save.resource_request.min_replica = 0

router_1.create_version(new_router_config_to_save)
new_undeployed_version = turing.router.config.router_version.RouterVersion.create(new_router_config_to_deploy,
router_id=1)

# Notice that the latest router version is undeployed (Turing has created the new version without deploying it)
versions = router_1.list_versions()
for v in versions:
print(v)

# Alternatively, you may also do the following to create a new version for your router without deploying it.
new_undeployed_version = turing.router.config.router_version.RouterVersion.create(new_router_config_to_save,
router_id=1)


if __name__ == '__main__':
import fire
Expand Down
5 changes: 3 additions & 2 deletions sdk/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -334,12 +334,13 @@ def generic_ensembler_docker_config(generic_resource_request, generic_env_var):


@pytest.fixture
def generic_ensembler_pyfunc_config(generic_resource_request):
def generic_ensembler_pyfunc_config(generic_resource_request, generic_env_var):
return turing.generated.models.EnsemblerPyfuncConfig(
project_id=77,
ensembler_id=11,
resource_request=generic_resource_request,
timeout="500ms"
timeout="500ms",
env=[generic_env_var]
)


Expand Down
19 changes: 17 additions & 2 deletions sdk/tests/router/config/router_ensembler_config_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ def test_create_router_ensembler_config(id, type, standard_config, docker_config
).to_open_api()
assert actual == request.getfixturevalue(expected)


@pytest.mark.parametrize(
"project_id,ensembler_id,resource_request,timeout,expected", [
"project_id,ensembler_id,resource_request,timeout,env,expected", [
pytest.param(
77,
11,
Expand All @@ -78,6 +79,11 @@ def test_create_router_ensembler_config(id, type, standard_config, docker_config
memory_request='512Mi'
),
"500ms",
[
EnvVar(
name="env_name",
value="env_val")
],
"generic_pyfunc_router_ensembler_config"
)
])
Expand All @@ -86,6 +92,7 @@ def test_create_pyfunc_router_ensembler_config(
ensembler_id,
resource_request,
timeout,
env,
expected,
request
):
Expand All @@ -94,12 +101,13 @@ def test_create_pyfunc_router_ensembler_config(
ensembler_id=ensembler_id,
resource_request=resource_request,
timeout=timeout,
env=env
).to_open_api()
assert actual == request.getfixturevalue(expected)


@pytest.mark.parametrize(
"project_id,ensembler_id,resource_request,timeout,expected", [
"project_id,ensembler_id,resource_request,timeout,env,expected", [
pytest.param(
77,
11,
Expand All @@ -110,6 +118,11 @@ def test_create_pyfunc_router_ensembler_config(
memory_request='512Mi'
),
"500ks",
[
EnvVar(
name="env_name",
value="env_val")
],
ApiValueError
)
])
Expand All @@ -118,6 +131,7 @@ def test_create_pyfunc_router_ensembler_config_with_invalid_timeout(
ensembler_id,
resource_request,
timeout,
env,
expected
):
with pytest.raises(expected):
Expand All @@ -126,6 +140,7 @@ def test_create_pyfunc_router_ensembler_config_with_invalid_timeout(
ensembler_id=ensembler_id,
resource_request=resource_request,
timeout=timeout,
env=env
).to_open_api()


Expand Down
5 changes: 5 additions & 0 deletions sdk/turing/generated/model/ensembler_pyfunc_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
)

def lazy_import():
from turing.generated.model.env_var import EnvVar
from turing.generated.model.resource_request import ResourceRequest
globals()['EnvVar'] = EnvVar
globals()['ResourceRequest'] = ResourceRequest


Expand Down Expand Up @@ -86,6 +88,7 @@ def openapi_types():
'ensembler_id': (int,), # noqa: E501
'resource_request': (ResourceRequest,), # noqa: E501
'timeout': (str,), # noqa: E501
'env': ([EnvVar],), # noqa: E501
}

@cached_property
Expand All @@ -98,6 +101,7 @@ def discriminator():
'ensembler_id': 'ensembler_id', # noqa: E501
'resource_request': 'resource_request', # noqa: E501
'timeout': 'timeout', # noqa: E501
'env': 'env', # noqa: E501
}

_composed_schemas = {}
Expand Down Expand Up @@ -152,6 +156,7 @@ def __init__(self, project_id, ensembler_id, resource_request, timeout, *args, *
Animal class but this time we won't travel
through its discriminator because we passed in
_visited_composed_classes = (Animal,)
env ([EnvVar]): [optional] # noqa: E501
"""

_check_type = kwargs.pop('_check_type', True)
Expand Down
17 changes: 16 additions & 1 deletion sdk/turing/router/config/router_ensembler_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ def pyfunc_config(self, pyfunc_config: turing.generated.models.EnsemblerPyfuncCo
elif isinstance(pyfunc_config, dict):
pyfunc_config["resource_request"] = \
turing.generated.models.ResourceRequest(**pyfunc_config["resource_request"])
pyfunc_config["env"] = [turing.generated.models.EnvVar(**env_var) for env_var in pyfunc_config["env"]]
self._pyfunc_config = turing.generated.models.EnsemblerPyfuncConfig(
**pyfunc_config
)
Expand Down Expand Up @@ -127,19 +128,22 @@ def __init__(self,
project_id: int,
ensembler_id: int,
timeout: str,
resource_request: ResourceRequest):
resource_request: ResourceRequest,
env: List['EnvVar']):
"""
Method to create a new Pyfunc ensembler

:param project_id: project id of the current project
:param ensembler_id: ensembler_id of the ensembler
:param resource_request: ResourceRequest instance containing configs related to the resources required
:param timeout: request timeout which when exceeded, the request to the ensembler will be terminated
:param env: environment variables required by the container
"""
self.project_id = project_id
self.ensembler_id = ensembler_id
self.resource_request = resource_request
self.timeout = timeout
self.env = env
super().__init__(type="pyfunc")

@property
Expand Down Expand Up @@ -174,12 +178,23 @@ def timeout(self) -> str:
def timeout(self, timeout: str):
self._timeout = timeout

@property
def env(self) -> List['EnvVar']:
return self._env

@env.setter
def env(self, env: List['EnvVar']):
self._env = env

def to_open_api(self) -> OpenApiModel:
assert all(isinstance(env_var, EnvVar) for env_var in self.env)

self.pyfunc_config = turing.generated.models.EnsemblerPyfuncConfig(
project_id=self.project_id,
ensembler_id=self.ensembler_id,
resource_request=self.resource_request.to_open_api(),
timeout=self.timeout,
env=[env_var.to_open_api() for env_var in self.env],
)
return super().to_open_api()

Expand Down
1 change: 0 additions & 1 deletion sdk/turing/router/config/router_version.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from __future__ import annotations

import turing
import turing.generated.models
import dataclasses
from enum import Enum

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React, { useContext } from "react";
import { EuiDescriptionList } from "@elastic/eui";
import EnsemblersContext from "../../../../../providers/ensemblers/context";

export const PyFuncRefConfigTable = ({ config: { ensembler_id } }) => {
export const PyFuncConfigTable = ({ config: { ensembler_id } }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this for consistency

const { ensemblers } = useContext(EnsemblersContext);

const ensembler_name = Object.values(ensemblers)
Expand Down
Loading