Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow training a model on multiple annotations #8071

Merged
merged 22 commits into from
Sep 25, 2024

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Sep 11, 2024

URL of deployed dev instance (used for testing):

Steps to test:

  • go to admin > ai models
  • click on the "train new model" button
  • paste this CSV
66e2e04b010000df00b3f6fa
https://multiannotraining.webknossos.xyz/annotations/66e2dfe6010000ab00b3f6f7
https://multiannotraining.webknossos.xyz/annotations/66e2e06d010000ad00b3f6ff
https://multiannotraining.webknossos.xyz/annotations/66e2e08c010000ad00b3f702
https://multiannotraining.webknossos.xyz/annotations/66e2e0d6010000ac00b3f706
  • click "add"
  • use the form
  • submit it (it will fail because there is no worker configured on the dev instance). however, you can use the network tab to inspect the sent payload.

image
image

  • also open an annotation and use the train-model dialog from there to ensure that it still works
  • special care needs to be taken for the volume layer name, because we usually use the tracing id as the name and then have a human-readable name for the UI. however, the worker always wants the human readable one and never the tracing id.

Issues:


  • Updated changelog
  • Updated documentation if applicable
  • Removed dev-only changes like prints and application.conf edits

@philippotto philippotto self-assigned this Sep 11, 2024
@philippotto philippotto changed the title [WIP] Allow training a model on multiple annotations Allow training a model on multiple annotations Sep 12, 2024
@@ -240,7 +240,7 @@ const defaultState: OxalisState = {
showVersionRestore: false,
showDownloadModal: false,
showPythonClientModal: false,
aIJobModalState: "invisible",
aIJobModalState: "neuron_inferral",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
aIJobModalState: "neuron_inferral",
aIJobModalState: "invisible",

@philippotto philippotto marked this pull request as ready for review September 12, 2024 12:59
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for implementing this, I'm looking forward to use the feature! Very thorough implementation 👍

Notes from testing:

  • Either the placeholder or the validation for the csv modal is wrong. The placeholder indicates that the color and segmentation layer names can be supplied, but the validation complains that only the url/id should be given.
    placeholder
  • The csv and the training modal are closed when clicking onto the back drop. The second one (and arguably the first one as well) should not do that, because it makes it to easy to lose entered information.
  • Whether the modal is triggered from within an annotation or from the models list, allows to select different layers (i.e., the fallback layer):
    within_annotation
    from_models_list

@normanrz
Copy link
Member

@MichaelBuessemeyer Could you please carry this over the finish line?

@MichaelBuessemeyer MichaelBuessemeyer self-assigned this Sep 18, 2024
@MichaelBuessemeyer MichaelBuessemeyer removed their request for review September 18, 2024 09:31
@MichaelBuessemeyer
Copy link
Contributor

Ok here we go :)
I hope I implemented everything you mentioned or commented it. Could you please do some retesting & re-reviewing of the changes @daniel-wer 🙏?

Either the placeholder or the validation for the csv modal is wrong. The placeholder indicates that the color and segmentation layer names can be supplied, but the validation complains that only the url/id should be given.

I removed the placeholder as the current code does not support directly supplying the color and segmentation layer to use for an annotation. If this is a helpful feature I'd prefer to do this as a follow-up :)

Whether the modal is triggered from within an annotation or from the models list, allows to select different layers (i.e., the fallback layer):

I don't understand what the issue is with the option to be able to select layers. An annotation might have multiple volume layers and thus the segmentation data that should be used is most of the time ambiguous. Same goes for color layers.

@daniel-wer
Copy link
Member

I don't understand what the issue is with the option to be able to select layers. An annotation might have multiple volume layers and thus the segmentation data that should be used is most of the time ambiguous. Same goes for color layers.

What I wanted to express is that the layer options offered to the user are different ones, depending on where the modal is opened. If you look at the screenshots, one time there are two options offered, and the other time there are three options, although I specified the same annotation. This should not be the case.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking over this PR!

Please also see my previous comment regarding the testing feedback. I can no longer test this on the dev instance, because the AI modal cannot be opened from within an annotation (Philipp patched the code to open the modal automatically, but you already removed that). Please test this locally or let me know if you don't know what I mean :)

function areBoundingBoxesValid(userBoundingBoxes: UserBoundingBox[] | undefined): {
valid: boolean;
reason: string | null;
function areAllAnnotationsInvalid<T extends HybridTracing | APIAnnotation>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The All seems to be incorrect and can probably simply be dropped

.map(
(layer) =>
getTracingForAnnotationType(annotation, layer) as Promise<ServerVolumeTracing>,
),
);
// TODO: make bboxs a member again
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ups, forgot to remove this. I already implemented it this way.

}
}

console.log(volumeTracings); // TODOM remove
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, sorry I sometimes tend to skip reading comments 🙈

frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx Outdated Show resolved Hide resolved
Comment on lines 480 to 493
const volumeTracings = volumeServerTracings.map((tracing) =>
serverVolumeToClientVolumeTracing(tracing),
);
let userBoundingBoxes = volumeTracings[0]?.userBoundingBoxes;
if (!userBoundingBoxes) {
const skeletonLayer = annotation.annotationLayers.find(
(layer) => layer.typ === "Skeleton",
);
if (skeletonLayer) {
const skeletonTracing = await getTracingForAnnotationType(annotation, skeletonLayer);
userBoundingBoxes = convertUserBoundingBoxesFromServerToFrontend(
skeletonTracing.userBoundingBoxes,
);
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment here that user bounding boxes are always saved in all annotation layers, but if there is no volume annotation layer, the skeleton layer is checked. At least that's how I interpret it, but I was a bit confused at first :)


const { TextArea } = Input;
const FormItem = Form.Item;

export type AnnotationInfoForAIJob<GenericAnnotation> = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that I also added this comment. It hopefully explains why all these additional properties (volumeTracings, userBoundingBoxes, volumeTracingResolutions) are needed. Please tell me your opinion on whether you understand what I want to express and maybe on wording improvements 🙏

@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Sep 24, 2024

What I wanted to express is that the layer options offered to the user are different ones, depending on where the modal is opened. If you look at the screenshots, one time there are two options offered, and the other time there are three options, although I specified the same annotation. This should not be the case.

Oh good finding 🎉

I think I fixed it now by simply filtering out volume data layers of the dataset that work as a fallback layer for one of the volume tracing layers.

The rest of your awesome feedback 🦶 🦶 🔙 should also be applied. 👯

Co-authored-by: Daniel <daniel.werner@scalableminds.com>
- add comment about why AnnotationInfoForAIJob is so complex
- filter out dataset segmentation layers that are used as fallback layers of other volume tracing layers of an annotation
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for polishing this. I'm looking forward to using this :)

frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx Outdated Show resolved Hide resolved
@daniel-wer
Copy link
Member

As a note, I've created #8097 for two smaller followup issues that came to my mind, but I didn't want to further bloat and delay this PR :) After merging this, we should test it in production, extend the mentioned issue with any other shortcomings we notice, and then implement the followups 🚀

@MichaelBuessemeyer MichaelBuessemeyer merged commit 4d2261d into master Sep 25, 2024
2 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the multi-anno-training branch September 25, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start train_model with multiple annotations
4 participants