Skip to content

Commit

Permalink
fix: Prevent calls to ensembling job service/controllers if batch ens…
Browse files Browse the repository at this point in the history
…embling is deactivated (#356)

* Add check to ensure batch ensembling jobs table is not shown when inactive

* Add additional checks to prevent calls to the ensembling job svc if inactive

* Fix unit tests
  • Loading branch information
deadlycoconuts authored Aug 4, 2023
1 parent ec567ae commit 9262b6d
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 2 deletions.
10 changes: 10 additions & 0 deletions api/turing/api/ensemblers_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,11 @@ func (c EnsemblersController) checkCurrentRouterVersion(options EnsemblersPathOp
}

func (c EnsemblersController) checkActiveEnsemblingJob(options EnsemblersPathOptions) (int, error) {
// Batch ensembling is not enabled
if c.EnsemblingJobService == nil {
return http.StatusOK, nil
}

ensemblingJobActiveOption := service.EnsemblingJobListOptions{
EnsemblerID: options.EnsemblerID,
Statuses: []models.Status{
Expand Down Expand Up @@ -298,6 +303,11 @@ func (c EnsemblersController) deleteInactiveRouterVersion(options EnsemblersPath
return http.StatusOK, nil
}
func (c EnsemblersController) deleteInactiveEnsemblingJob(options EnsemblersPathOptions) (int, error) {
// Batch ensembling is not enabled
if c.EnsemblingJobService == nil {
return http.StatusOK, nil
}

ensemblingJobInactiveOption := service.EnsemblingJobListOptions{
EnsemblerID: options.EnsemblerID,
Statuses: []models.Status{
Expand Down
32 changes: 31 additions & 1 deletion api/turing/api/ensemblers_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -889,6 +889,36 @@ func TestEnsemblerController_DeleteEnsembler(t *testing.T) {
},
expected: InternalServerError("failed to delete the ensembler", "failed to delete"),
},
"success | batch ensembling is not enabled": {
vars: RequestVars{
"project_id": {"2"},
"ensembler_id": {"2"},
},
ensemblerSvc: func() service.EnsemblersService {
ensemblerSvc := &mocks.EnsemblersService{}
ensemblerSvc.
On("FindByID", models.ID(2), service.EnsemblersFindByIDOptions{
ProjectID: models.NewID(2),
}).
Return(original, nil)
ensemblerSvc.
On("Delete", original).
Return(nil)
return ensemblerSvc
},
routerVersionsSvc: func() service.RouterVersionsService {
routerVersionSvc := &mocks.RouterVersionsService{}
routerVersionSvc.On("ListRouterVersionsWithFilter", mock.Anything).Return([]*models.RouterVersion{}, nil)

return routerVersionSvc
},
mlflowSvc: func() mlflow.Service {
mlflowSvc := &mlflowMock.Service{}
mlflowSvc.On("DeleteExperiment", mock.Anything, "1", true).Return(nil)
return mlflowSvc
},
expected: Ok(models.ID(2)),
},
"success": {
vars: RequestVars{
"project_id": {"2"},
Expand Down Expand Up @@ -951,7 +981,7 @@ func TestEnsemblerController_DeleteEnsembler(t *testing.T) {
ensemblingJobSvc = tt.ensemblingJobSvc()
}
var routerVersionsSvc service.RouterVersionsService
if tt.ensemblingJobSvc != nil {
if tt.routerVersionsSvc != nil {
routerVersionsSvc = tt.routerVersionsSvc()
}

Expand Down
7 changes: 6 additions & 1 deletion ui/src/ensembler/components/modal/DeleteEnsemblerModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { isEmpty } from "../../../utils/object";
import { ListEnsemblingJobsForEnsemblerTable } from "../table/ListEnsemblingJobsForEnsemblerTable";
import { ListRouterVersionsForEnsemblerTable } from "../table/ListRouterVersionsForEnsemblerTable";
import { EuiFieldText } from "@elastic/eui";
import {useConfig} from "../../../config";

export const DeleteEnsemblerModal = ({
onSuccess,
Expand All @@ -23,6 +24,10 @@ export const DeleteEnsemblerModal = ({
const [deleteConfirmation, setDeleteConfirmation] = useState('')
const [ensembler = {}, openModal, closeModal] = useEnsemblerModal(closeModalRef);

const {
appConfig: { batchEnsemblingEnabled },
} = useConfig();

useEffect(() => {
// if ensembler is used by one of the component, immediately set can delete ensembler to false
setCanDeleteEnsembler(!(ensemblerUsedByActiveEnsemblingJob || ensemblerUsedByActiveRouterVersion || ensemblerUsedByCurrentRouterVersion))
Expand Down Expand Up @@ -93,7 +98,7 @@ export const DeleteEnsemblerModal = ({
</div>
)}
{/* Only show The Ensembling Table if ensembler is not used by current router version */}
{!ensemblerUsedByCurrentRouterVersion && (
{!ensemblerUsedByCurrentRouterVersion && batchEnsemblingEnabled && (
<ListEnsemblingJobsForEnsemblerTable
projectID={ensembler.project_id}
ensemblerID={ensembler.id}
Expand Down

0 comments on commit 9262b6d

Please sign in to comment.