added backend implementation for model templating #1419
Conversation
backend/src/services/model.ts
Outdated
| checkModelRestriction(model) | ||
| const template = await getModelById(user, templateId) | ||
| checkModelRestriction(template) | ||
|
|
There was a problem hiding this comment.
We should check that modelId !== templateId
backend/src/services/model.ts
Outdated
| if (!modelAuth.success) { | ||
| throw Forbidden(modelAuth.info, { userDn: user.dn, modelId }) | ||
| } |
There was a problem hiding this comment.
This is already checked by the _setModelCard function
backend/src/services/model.ts
Outdated
| const model = await getModelById(user, modelId) | ||
| checkModelRestriction(model) | ||
| const template = await getModelById(user, templateId) | ||
| checkModelRestriction(template) |
There was a problem hiding this comment.
I'm not sure this check is necessary
There was a problem hiding this comment.
I think it's necessary to check that new model is not a mirrored model - there could be an instance that someone makes a mirrored model but doesn't create a model card, then tries to base it off a template.
As we are not changing the metadata of the template, however, I think we can safely remove this check.
backend/src/services/model.ts
Outdated
| } | ||
|
|
||
| if (!template.card) { | ||
| throw BadReq('The template model is missing a modelcard', { modelId }) |
There was a problem hiding this comment.
I think the templateId should be given as context
backend/src/services/model.ts
Outdated
| throw Forbidden(templateAuth.info, { userDn: user.dn, modelId }) | ||
| } | ||
|
|
||
| if (!template.card) { |
There was a problem hiding this comment.
By default card seems to be set as an empty object so this won't catch the error. Instead you can use !template.card?.schemaId
There was a problem hiding this comment.
When you first create a model card doesn't exist - I'm not sure where you're seeing it being set as an empty object, but if it is somehow being set let me know!
| throw BadReq('The model and template ID must be different', { modelId, templateId }) | ||
| } | ||
| const model = await getModelById(user, modelId) | ||
| checkModelRestriction(model) |
There was a problem hiding this comment.
Do we need to checkModelRestriction for the model being used as a template also?
There was a problem hiding this comment.
I don't think so. All it does is check to see if it's "mirrored" which is basically read only. As we're not editing the template in any way, I think it's fine to use it if it's read only.
backend/src/services/model.ts
Outdated
| // Check to make sure user can access the template. We already check for the model auth later on in _setModelCard | ||
| const templateAuth = await authorisation.model(user, template, ModelAction.View) | ||
| if (!templateAuth.success) { | ||
| throw Forbidden(templateAuth.info, { userDn: user.dn, modelId }) |
There was a problem hiding this comment.
Shouldn't this be reporting the templateId?
throw Forbidden(templateAuth.info, { userDn: user.dn, modelId: templateId })
| ): Promise<ModelCardRevisionDoc> { | ||
| throw NotImplemented({}, 'This feature is not yet implemented') | ||
| if (modelId === templateId) { | ||
| throw BadReq('The model and template ID must be different', { modelId, templateId }) |
There was a problem hiding this comment.
We should have a unit test that covers this
backend/src/services/model.ts
Outdated
| // Check to make sure user can access the template. We already check for the model auth later on in _setModelCard | ||
| const templateAuth = await authorisation.model(user, template, ModelAction.View) | ||
| if (!templateAuth.success) { | ||
| throw Forbidden(templateAuth.info, { userDn: user.dn, modelId }) |
There was a problem hiding this comment.
We should have a unit test that covers this
backend/test/services/model.spec.ts
Outdated
| @@ -311,4 +312,17 @@ describe('services > model', () => { | |||
| ) | |||
| expect(modelMocks.save).not.toBeCalled() | |||
| }) | |||
|
|
|||
| test('crateModelCardFromTemplate > requesting to use a template without a model card will throw an error', async () => { | |||
There was a problem hiding this comment.
We should also have a "happy day" test
| if (!template.card?.schemaId) { | ||
| throw BadReq('The template model is missing a model card', { modelId, templateId }) | ||
| } | ||
| const revision = await _setModelCard(user, modelId, template.card.schemaId, 1, template.card.metadata) |
There was a problem hiding this comment.
Is there anything to stop a user from calling this with a modelId that references a model that already has model card versions? If this were to happen then this line would set the first version to be the model card of the template and all later versions would be from the pre-existing model card.
No description provided.