Add inferencing routes #1097
Add inferencing routes #1097
Conversation
backend/src/services/v2/inference.ts
Outdated
| throw error | ||
| } | ||
|
|
||
| const auth = await authorisation.model(user, model, ModelAction.Update) |
There was a problem hiding this comment.
It's a good idea to move this check as high up as possible to avoid any unnecessary computation should the user not be authorised. At the very least, it should definitely be above the saving of the mongo document.
| export const getInferenceSchema = z.object({ | ||
| params: z.object({ | ||
| modelId: z.string({ | ||
| required_error: 'Must specify model id as param', |
There was a problem hiding this comment.
We've started to move away from custom required errors here and just use the standard zod error
| @@ -131,6 +137,11 @@ export abstract class BaseAuditConnector { | |||
| abstract onDeleteSchema(req: Request, schemaId: string) | |||
| abstract onUpdateSchema(req: Request, schema: SchemaDoc) | |||
|
|
|||
| abstract onCreateInference(req: Request, inference: InferenceDoc) | |||
There was a problem hiding this comment.
When auditing it would be useful to include enough information so that the resource (in this case the inference settings) are identifiable and retrievable. Judging by your get inference route, I'm guessing this would also require auditing the model ID?
backend/src/services/v2/inference.ts
Outdated
| } | ||
|
|
||
| const updatedInference = await Inference.findOneAndUpdate( | ||
| { modelId: inference.modelId, image: inference.image, tag: inference.tag }, |
There was a problem hiding this comment.
What would happen here if you tried to update the inference to be the same as an existing one?
There was a problem hiding this comment.
Also, should you allow the user to update the model ID on the inference setting? You've only checked their permissions on the existing model and the route is specific for the existing model ID. I would assume that they would have to delete the existing one and make another request to create an inference service on another model.
This reverts commit 12347c5.
| export const postInferenceSchema = z.object({ | ||
| params: z.object({ | ||
| modelId: z.string({ | ||
| required_error: 'Must specify model id as param', |
backend/src/services/inference.ts
Outdated
| throw NotFound(`The requested inferencing service was not found.`, { modelId, image, tag }) | ||
| } | ||
|
|
||
| const auth = await authorisation.model(user, model, ModelAction.View) |
There was a problem hiding this comment.
This technically isn't requires because when you getModelById() this authorisation check takes place already. The method getInferenceByModel() seems to already to do this and rely on the authorisation check in getModelById(). I think you could make a case for keeping the identical authorisation check in this service as it adds clarity but be consistent with which option you decide.
There was a problem hiding this comment.
Some missing test cases here, use the vitest coverage report to help add the missing tests
No description provided.