Implement the DELETE model endpoint. #211
Conversation
martacarbone
force-pushed
the
ai-delete-endpoint
branch
from
1ca469e to
948a8d0
Compare
internal/orchestrator/models.go
Outdated
| idProvider, cfg) | ||
|
|
||
| if err != nil { | ||
| return nil, errors.New("error retrieving applications.") |
There was a problem hiding this comment.
i would retutn the internal err
| render.EncodeResponse(w, http.StatusNotFound, models.ErrorResponse{Details: err.Error()}) | ||
| case errors.Is(err, orchestrator.ErrConflict): | ||
| render.EncodeResponse(w, http.StatusConflict, models.ErrorResponse{Details: err.Error()}) |
There was a problem hiding this comment.
return an errorResponse also here to have json formatted
| } | ||
|
|
||
| if res.IsPreinstalled { | ||
| return fmt.Errorf("model %s is a pre-installed model: %w", id, ErrConflict) |
There was a problem hiding this comment.
I would return a sentinel error only
| return fmt.Errorf("model %s is a pre-installed model: %w", id, ErrConflict) | |
| return ErrCannotRemoveModel |
| return nil | ||
| } | ||
|
|
||
| func isModelReferencedInApp(ctx context.Context, dockerClient command.Cli, cfg config.Configuration, idProvider *app.IDProvider, modelId string) ([]string, error) { |
There was a problem hiding this comment.
I would return the list of apps that have at least one reference to that model.
It is possible that an app has multiple bricks pointing to the same model
| if running != nil { | ||
| sb.WriteString(fmt.Sprintf("The model %s is in use by the %s app. Stop the application before removing the model", id, running.Name)) | ||
| } |
There was a problem hiding this comment.
We could separate the error.
-if running => return the error that the mode is used by a running app.
-if multiple references => return error that a model is referenced by apps.
| return fmt.Errorf("%s: %w", sb.String(), ErrConflict) | ||
| } | ||
|
|
||
| if res.ModelFolderPath == nil { |
There was a problem hiding this comment.
This should never happen, because every pre-installed must have a path.
We could log the warning but do not raise an error.
| ) | ||
|
|
||
| func AIModelDelete(ctx context.Context, dockerClient command.Cli, cfg config.Configuration, modelsIndex *modelsindex.ModelsIndex, id string, idProvider *app.IDProvider, force bool) (err error) { | ||
| res, found := modelsIndex.GetModelByID(id) |
There was a problem hiding this comment.
I would move the removal of the model inside the models-index.go since it will be used also by the CLI command.
3 participants
Motivation
This PR adds a deletion endpoint for models.
To ensure safety, a model is removed only if it is not in use by a running application and is not referenced by any other models.
Change description
Additional Notes
Reviewer checklist
main.