235 annotation guideline backend annotation controllers - #1533
dbrembilla wants to merge 6 commits into
Conversation
Introduce validate() method on the AnnotationGuideline model. Add a validation step when generating annotation or changing shapes via AnnotationControllers, when attaching AnnotationLabelControllers, adds a filtering step for labels in LabelBotService. Add a check for `annotation_guideline_id` to requests. Add Tests.
…ape for video annotations
mzur
left a comment
mzur
left a comment
There was a problem hiding this comment.
I didn't do a detailed review yet because there are some general issues that have to be addressed first (see below). This affects all controllers; I didn't add a comment everywhere.
Also, did you intentionally leave out changes required for Largo? Will you do these in a separate PR?
This needs a lot of care because it requires changes in the most important code paths of BIIGLE. Do you still have time for this? Otherwise I can take this over, too.
| $labelId = $input['label_id']; | ||
|
|
||
| if (!is_null($annotationGuideline) && !$annotationGuideline->validate($labelId, $shapeId)) { | ||
| abort(422, "Annotation uncompatible with enforced Annotation Guideline"); |
There was a problem hiding this comment.
This must be more detailed. Users need to know if the shape or the label was wrong and which annotation was invalid (the index in the input array).
In general, this validation step should be performed in the StoreImageAnnotations request class.
| $shapeId = $input['shape_id']; | ||
| $labelId = $input['label_id']; | ||
|
|
||
| if (!is_null($annotationGuideline) && !$annotationGuideline->validate($labelId, $shapeId)) { |
There was a problem hiding this comment.
Also this does not fail if a project has an enforced guideline but the client did not pass the guideline ID, right? If an annotation should be created and all the parent projects have enforced guidelines, the request must specify one of the guidelines.
| if (!is_null($annotationGuideline) && !$annotationGuideline->validate($label->id, $annotation->shape_id)) { | ||
| abort(422, "Annotation uncompatible with enforced Annotation Guideline"); | ||
| } |
There was a problem hiding this comment.
Same here. Validation should be done in the request, it needs a better error message and it should fail if all projects have an enforced guideline and none were chosen.
| $shapeId = $request->input('shape_id', $annotation->shape_id); | ||
|
|
||
| $annotationGuideline = null; | ||
| if ($request->has('annotation_guideline_id')) { |
| $annotationGuideline = null; | ||
| if (($annotationGuidelineId = $input['annotation_guideline_id'] ?? null) !== null) { | ||
| /** @phpstan-ignore nullCoalesce.offset */ | ||
| $annotationGuidelines[$annotationGuidelineId] ??= AnnotationGuideline::findOrFail($annotationGuidelineId); |
There was a problem hiding this comment.
You don't check if the guideline actually belongs to the project of the annotation.
| $annotations = collect($request->all())->map(function ($input) { | ||
| //Avoid having to find guideline repeatedly | ||
| $annotationGuidelines = []; | ||
| $annotations = collect($request->all())->map(function ($input) use ($annotationGuidelines) { |
There was a problem hiding this comment.
$annotationGuidelines is called per-value during the map so the changes do not persist across method calls. Use:
| $annotations = collect($request->all())->map(function ($input) use ($annotationGuidelines) { | |
| $annotations = collect($request->all())->map(function ($input) use (&$annotationGuidelines) { |
| try { | ||
| $topNLabels = $this->performVectorSearch($featureVector, $treeIds, $model); | ||
| if (!is_null($annotationGuideline)) { | ||
| $topNLabels = array_filter($topNLabels, fn ($labelId) => $annotationGuideline->validate($labelId, $shapeId)); |
There was a problem hiding this comment.
Ideally the vector search could take the allowed shape and labels into account right away, so users can still get 3 suggestions (just more unlikely) instead of 3 minus the disallowed ones.
| $request->validate([ | ||
| 'shape_id' => 'required_without:points|integer|exists:shapes,id', | ||
| 'points' => 'required_without:shape_id|array', | ||
| ]); |
There was a problem hiding this comment.
annotation_guideline_id is not validated at all.
|
Hi @mzur
Yes I did! This was already quite massive and did not want to miss important aspects within BIIGLE because, as you said, it is quite critical.
I don't think I will have time to look at it for at least a month between vacation days and focus. Maybe at the end of October I will have time to look deeper into it. But if you have time, you can of course do! |
2 participants
This PR adds, as specified in PR #1390, a validate method for
AnnotationGuidelineas well as the backend forAnnotationsControllersandAnnotationLabelControllers. Adds tests for each of them, multiple edge cases.