Skip to content

Commit

Permalink
fix(ui): Fix negative values shown on the replicas slider when an env…
Browse files Browse the repository at this point in the history
… is selected during model deployment (#549)

# Description
When deploying a model, selecting an environment where that model has
already been deployed in would cause the initial min replica value to
appear as -1, which is an invalid value:


https://github.com/caraml-dev/merlin/assets/36802364/465d433a-1e4d-4eb7-ab2d-27f082575241

Besides being invalid, the initial min replica value should also have
been set as the value found in the previous deployment. The same should
be expected for the max replica value, which from the video above, is
set as 0 initially, instead of the value found in the previous
deployment.

If an environment in which the model has not yet been deployed is
selected instead, we also notice incorrect initial min and max replica
values - they are set as 0 even though their default values are
configured as non-zero (these values can usually be configured in the
`default_deployment_config` schema of the `environments` config file,
just as how it has been done
[here](https://github.com/caraml-dev/merlin/blob/0a79211db6ed60b266ee75cf03088b4f22eb037d/environment.yaml#L19)).


https://github.com/caraml-dev/merlin/assets/36802364/6fcd03f8-9a54-4a8a-a56c-c6a1748de658

This PR thus addresses this bug whereby the min/max replica values are
unexpectedly set to -1/0 or 0/0, instead of the using those values from
a previous deployment or from the default values configured in the
environments file.

# Fix
The fix mainly involves triggering the effects and `onChange` handlers
for the `resourcesConfig` object **_only when an environment has been
selected_** (this means that the `environment` or `maxAllowedReplica`
variables are non-null and non-undefined). This ensures that the
`resourcesConfig` object does not get modified unexpectedly when an
environment is not selected.

New behaviour after the fix has been implemented:


https://github.com/caraml-dev/merlin/assets/36802364/28c513ba-c684-40fe-a6d0-b9a33e672832


# Modifications
- `ui/src/pages/version/components/forms/DeployModelVersionForm.js` -
Change the default value of `maxAllowedReplica` from `0` to `undefined`
when an environment is not selected
- `ui/src/pages/version/components/forms/components/ResourcesPanel.js` -
Add additional checks to effects, onChange handlers, and values to
display when an environment is not selected

# Checklist
- [x] Added PR label
- [ ] Added unit test, integration, and/or e2e tests
- [x] Tested locally
- [ ] Updated documentation
- [ ] Update Swagger spec if the PR introduce API changes
- [ ] Regenerated Golang and Python client if the PR introduces API
changes

# Release Notes
```release-note
NONE
```
  • Loading branch information
deadlycoconuts authored Mar 8, 2024
1 parent 0a79211 commit 82fe2bf
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export const DeployModelVersionForm = ({
if (data.environment_name !== "") {
return environments.find((env) => env.name === data.environment_name).max_allowed_replica;
}
return 0;
return undefined;
});

const mainSteps = [
Expand Down
24 changes: 14 additions & 10 deletions ui/src/pages/version/components/forms/components/ResourcesPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,14 @@ export const ResourcesPanel = ({
setGpuValueOptions([{ value: "None", inputDisplay: "None" }]);
}

if (resourcesConfig.min_replica > maxAllowedReplica) {
onChange("min_replica")(maxAllowedReplica - 1)
}
if (!!maxAllowedReplica) {
if (resourcesConfig.min_replica > maxAllowedReplica) {
onChange("min_replica")(maxAllowedReplica - 1)
}

if (resourcesConfig.max_replica > maxAllowedReplica) {
onChange("max_replica")(maxAllowedReplica)
if (resourcesConfig.max_replica > maxAllowedReplica) {
onChange("max_replica")(maxAllowedReplica)
}
}

}, [resourcesConfig, resourcesConfig.gpu_name, gpus, maxAllowedReplica, onChange]);
Expand Down Expand Up @@ -242,16 +244,18 @@ export const ResourcesPanel = ({
<EuiDualRange
fullWidth
min={0}
max={maxAllowedReplica}
max={maxAllowedReplica || 0}
showInput
showLabels
value={[
resourcesConfig.min_replica || 0,
resourcesConfig.max_replica || 0,
!!environment ? resourcesConfig.min_replica : 0,
!!environment ? resourcesConfig.max_replica : 0,
]}
onChange={([min_replica, max_replica]) => {
onChange("min_replica")(parseInt(min_replica));
onChange("max_replica")(parseInt(max_replica));
if (!!environment) {
onChange("min_replica")(parseInt(min_replica));
onChange("max_replica")(parseInt(max_replica));
}
}}
isInvalid={!!replicasError.length}
aria-label="autoscaling"
Expand Down

0 comments on commit 82fe2bf

Please sign in to comment.