Add option to train AI using Tasks and their bounding boxes #8310
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the AI model training functionality, focusing on enhancing the ability to use task-based annotations for training. The changes primarily involve updating type definitions from Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
620-620: Consider using an enum or constant for task state.The string literal
"Finished"for task state could be replaced with a type-safe constant or enum.Consider defining a constant or enum:
const TASK_STATE = { FINISHED: "Finished", } as const;Then use it in the code:
- const finishedAnnotations = annotations.filter(({ state }) => state === "Finished"); + const finishedAnnotations = annotations.filter(({ state }) => state === TASK_STATE.FINISHED);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx(2 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (6)
frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx (1)
15-18: LGTM! Type updates are consistent.The changes correctly update the type imports and usage from
AnnotationInfoForAIJobtoAnnotationInfoForAITrainingJob.Also applies to: 112-112
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (5)
50-50: LGTM! Import added for task support.The addition of
getAnnotationsForTaskimport aligns with the PR objective of supporting tasks in AI training.
59-64: LGTM! Clear documentation of task bounding box support.The comments effectively explain the rationale for including task bounding boxes and the relationship with user bounding boxes.
181-182: LGTM! Function parameters updated consistently.The type annotations in
TrainAiModelTabparameters have been correctly updated to useAnnotationInfoForAITrainingJob.
668-677: LGTM! Task bounding box handling.The code correctly adds the task's bounding box to the list of user bounding boxes with proper ID generation and naming.
690-694: LGTM! Clear warning for unfinished tasks.The code provides clear feedback when tasks without finished annotations are encountered.
| let isTask = true; | ||
| try { | ||
| const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl); | ||
| const finishedAnnotations = annotations.filter(({ state }) => state === "Finished"); | ||
| if (annotations.length > 0) { | ||
| annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id)); | ||
| } else { | ||
| unfinishedTasks.push(taskOrAnnotationIdOrUrl); | ||
| } | ||
| } catch (_e) { | ||
| isTask = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in task validation.
The catch block silently swallows all errors to determine if the input is a task. This could hide real API errors or other issues.
Consider this improvement:
- try {
- const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl);
- const finishedAnnotations = annotations.filter(({ state }) => state === "Finished");
- if (annotations.length > 0) {
- annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id));
- } else {
- unfinishedTasks.push(taskOrAnnotationIdOrUrl);
- }
- } catch (_e) {
- isTask = false;
- }
+ try {
+ const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl);
+ const finishedAnnotations = annotations.filter(({ state }) => state === "Finished");
+ if (annotations.length > 0) {
+ annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id));
+ } else {
+ unfinishedTasks.push(taskOrAnnotationIdOrUrl);
+ }
+ } catch (error) {
+ if (error.response?.status === 404) {
+ isTask = false;
+ } else {
+ throw new Error(`Failed to fetch task ${taskOrAnnotationIdOrUrl}: ${error.message}`);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let isTask = true; | |
| try { | |
| const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl); | |
| const finishedAnnotations = annotations.filter(({ state }) => state === "Finished"); | |
| if (annotations.length > 0) { | |
| annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id)); | |
| } else { | |
| unfinishedTasks.push(taskOrAnnotationIdOrUrl); | |
| } | |
| } catch (_e) { | |
| isTask = false; | |
| } | |
| let isTask = true; | |
| try { | |
| const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl); | |
| const finishedAnnotations = annotations.filter(({ state }) => state === "Finished"); | |
| if (annotations.length > 0) { | |
| annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id)); | |
| } else { | |
| unfinishedTasks.push(taskOrAnnotationIdOrUrl); | |
| } | |
| } catch (error) { | |
| if (error.response?.status === 404) { | |
| isTask = false; | |
| } else { | |
| throw new Error(`Failed to fetch task ${taskOrAnnotationIdOrUrl}: ${error.message}`); | |
| } | |
| } |
|
@daniel-wer I just noticed that the worker bucket & mag aligns the bounding boxes before using them. This is not done by the frontend checking mechanism. But this aligning correction might lead to different sizes of the bounding boxes and therefore the frontend assertion that bounding boxes have a multiple size of the smallest bounding box might no longer be correct. => Therefore I would also suggest doing the aligning in the frontend and then doing the bbox size assertion checking. Moreover, the currently only bboxes with an extent of < 10 in at least one direction are counted as too small. But according to https://scm.slack.com/archives/C03B6160ATY/p1736344267807119 this is too small. Therefore, I would also suggest to increase this limit. But, I am not familiar enough with the training code to tell what would be a better limit. I would need you're help here 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AiModelController.scala(1 hunks)conf/messages(1 hunks)frontend/javascripts/admin/tasktype/task_type_create_view.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/admin/tasktype/task_type_create_view.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~25-~25: You’ve already used the word ‘also’ once in your sentence, so using it again may be redundant.
Context: ...now also support inputting task ids and also considers their task bounding box. [#8310](https:...
(REDUNDANT_FILLER)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
app/controllers/AiModelController.scala (1)
156-156: LGTM! Error message key change improves clarity.The change from "model.nameInUse" to "aiModel.nameInUse" makes the error key more specific and consistent with the AI model domain.
conf/messages (1)
376-376: LGTM! Clear and consistent error message.The new error message follows the established format and provides clear guidance to users when they attempt to use a duplicate AI model name.
CHANGELOG.unreleased.md
Outdated
| @@ -22,6 +22,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released | |||
| - Renamed "resolution" to "magnification" in more places within the codebase, including local variables. [#8168](https://github.com/scalableminds/webknossos/pull/8168) | |||
| - Layer names are now allowed to contain `$` as special characters. [#8241](https://github.com/scalableminds/webknossos/pull/8241) | |||
| - Datasets can now be renamed and can have duplicate names. [#8075](https://github.com/scalableminds/webknossos/pull/8075) | |||
| - Starting an ai training job using multiple annotations now also support inputting task ids and also considers their task bounding box. [#8310](https://github.com/scalableminds/webknossos/pull/8310) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve changelog entry clarity and correctness.
The changelog entry contains grammatical issues and redundant wording. Consider this revision:
-- Starting an ai training job using multiple annotations now also support inputting task ids and also considers their task bounding box. [#8310](https://github.com/scalableminds/webknossos/pull/8310)
++ Starting an AI training job using multiple annotations now supports inputting task-IDs and considers their task bounding boxes. [#8310](https://github.com/scalableminds/webknossos/pull/8310)Changes made:
- Fixed subject-verb agreement ("supports")
- Capitalized "AI"
- Hyphenated "task-IDs"
- Removed redundant "also"
- Made "bounding box" plural to match multiple tasks
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Starting an ai training job using multiple annotations now also support inputting task ids and also considers their task bounding box. [#8310](https://github.com/scalableminds/webknossos/pull/8310) | |
| - Starting an AI training job using multiple annotations now supports inputting task-IDs and considers their task bounding boxes. [#8310](https://github.com/scalableminds/webknossos/pull/8310) |
🧰 Tools
🪛 LanguageTool
[style] ~25-~25: You’ve already used the word ‘also’ once in your sentence, so using it again may be redundant.
Context: ...now also support inputting task ids and also considers their task bounding box. [#8310](https:...
(REDUNDANT_FILLER)
|
Moreover, we could also check that enough annotations are provided. When training with multiple annotations, the training code seems to assume that at least 4 annotations are given by the user |
Good catch, please do so if it's not too complicated. Maybe also a warning should be shown if a bounding box is not mag-aligned. This would make it somewhat easier for users to understand why bounding box sizes might differ in mags > 1.
I think it should be fine to go with a minimum extent of 32 vx in the training mag. Do you agree @valentin-pinkau?
Mmh, I'm not sure I get that. I don't know of any minimum number of annotations assumed by the training code. Do you mean bounding boxes? If so, the |
Yes, I mean exactly that: The training process always died on me before actually starting the training because of a |
…into enable-taskbased-training
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
721-730: Consider extracting the color constant.The black color [0, 0, 0] for task bounding boxes could be defined as a named constant for better maintainability.
+const TASK_BOUNDING_BOX_COLOR = [0, 0, 0] as const; + if (annotation.task?.boundingBox) { const largestId = Math.max(...userBoundingBoxes.map(({ id }) => id)); userBoundingBoxes.push({ name: "Task Bounding Box", boundingBox: Utils.computeBoundingBoxFromBoundingBoxObject(annotation.task.boundingBox), - color: [0, 0, 0], + color: TASK_BOUNDING_BOX_COLOR, isVisible: true, id: largestId + 1, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (5)
CHANGELOG.unreleased.md (1)
25-25: LGTM! Clear and accurate changelog entry.The entry effectively communicates the new feature of supporting task IDs and task bounding boxes in AI training jobs.
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (4)
59-64: Well-documented changes!The comments clearly explain the handling of task bounding boxes and multiple bounding box sizes.
538-538: Robust bounding box validation implementation!The code correctly:
- Enforces minimum extent of 32 voxels per dimension
- Validates magnification alignment
- Provides clear warnings for misaligned boxes
Also applies to: 560-598, 617-626
743-747: Clear user feedback for unfinished tasks!The warning toast effectively communicates when tasks have no finished annotations.
669-681: 🛠️ Refactor suggestionImprove error handling for task validation.
The current implementation silently catches all errors to determine if the input is a task. This could mask real API errors.
Consider this improvement:
try { const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl, { showErrorToast: false, }); const finishedAnnotations = annotations.filter(({ state }) => state === "Finished"); if (annotations.length > 0) { annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id)); } else { unfinishedTasks.push(taskOrAnnotationIdOrUrl); } - } catch (_e) { + } catch (error) { + if (error.response?.status === 404) { isTask = false; + } else { + throw new Error(`Failed to fetch task ${taskOrAnnotationIdOrUrl}: ${error.message}`); + } }Likely invalid or redundant comment.
I just retested this by adding more and more bounding boxes to a single annotation. Once I reached 4 bounding boxes, the training no longer failed. Alternatively, enlarging a single bounding box also works. Not sure what happened in your case 🤔 The test_fraction is computed dynamically, but it is at most 0.3 and only goes down if the bounding boxes contain more than ~16M voxels at which point there should be more training samples than are needed to have at least one example in the validation set. However, what complicates this is that not the number of bounding boxes is critical here, but the number of training samples. Larger bounding boxes can contain more than one training sample. I think it would be very complicated to calculate this in the frontend and would therefore not do anything about it for now. If we see users hitting this issue we can reconsider this decision. |
|
@daniel-wer Can you do the review of this pr and the corresponding worker pr as you are already familiar with the issue? The worker pr is very small and should be easy to understand. |
|
Yes I'll happily do the reviews :) |
…ble-taskbased-training
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
27-27: Enhance changelog entry with implementation details.While the entry correctly describes the main change, consider adding important implementation details discussed in the PR:
- The validation of task bounding boxes before training
- The requirement for mag-aligned bounding boxes
- The minimum number of annotations needed for training
- Starting an AI training job using multiple annotations now supports inputting task-IDs and considers their task bounding boxes. [#8310](https://github.com/scalableminds/webknossos/pull/8310) + Starting an AI training job using multiple annotations now supports inputting task-IDs and considers their task bounding boxes. The system validates that bounding boxes are properly mag-aligned and ensures a minimum of four annotations for successful training. [#8310](https://github.com/scalableminds/webknossos/pull/8310)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/api/tasks.ts(1 hunks)frontend/javascripts/admin/tasktype/task_type_create_view.tsx(1 hunks)frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx(1 hunks)frontend/javascripts/oxalis/model/helpers/shader_editor.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/javascripts/oxalis/model/helpers/shader_editor.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/javascripts/admin/tasktype/task_type_create_view.tsx
- frontend/javascripts/admin/api/tasks.ts
- frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
665-692:⚠️ Potential issueImprove error handling in task validation.
The catch block silently swallows all errors to determine if the input is a task. This could hide real API errors or other issues.
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (3)
538-543: Consider extracting bounding box types to a separate file.The type definition for bounding box validation could be moved to a separate types file to improve code organization.
+// In types/bounding_box_types.ts +export type BoundingBoxWithTrainingInfo = UserBoundingBox & { + annotationId: string; + trainingMag: Vector3 | undefined; +}; -function checkBoundingBoxesForErrorsAndWarnings( - userBoundingBoxes: (UserBoundingBox & { - annotationId: string; - trainingMag: Vector3 | undefined; - })[], +function checkBoundingBoxesForErrorsAndWarnings( + userBoundingBoxes: BoundingBoxWithTrainingInfo[],
751-755: Consider adding more context to the warning message.The warning message could be more informative by explaining what action users should take.
- `The following tasks have no finished annotations: ${unfinishedTasks.join(", ")}`, + `The following tasks have no finished annotations and will be skipped: ${unfinishedTasks.join(", ")}. Please ensure these tasks are completed before including them for training.`,
785-785: Update placeholder text to be more descriptive.The current placeholder text could be more helpful by providing examples.
- placeholder="taskOrAnnotationIdOrUrl" + placeholder="Enter task IDs or annotation URLs (one per line)\nExample:\ntask_123\nannotation_456\nhttps://webknossos.org/annotations/789"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx(2 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/admin/voxelytics/ai_model_list_view.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (4)
59-64: LGTM! Clear and comprehensive documentation.The comments effectively explain the rationale behind handling multiple bounding box sizes, fallback data support, and task bounding boxes.
181-182: LGTM! Type signature updates.The type rename from
AnnotationInfoForAIJobtoAnnotationInfoForAITrainingJobbetter reflects its purpose.
560-606: LGTM! Robust bounding box alignment logic.The code correctly handles magnification alignment by:
- Checking if training magnification is available
- Aligning bounding boxes with the magnification
- Tracking and warning about any alignment adjustments
729-738: LGTM! Task bounding box integration.The code correctly:
- Checks for task bounding box presence
- Generates a unique ID for the new bounding box
- Adds it to the user bounding boxes with appropriate defaults
There was a problem hiding this comment.
Well done 👍 Testing also went well 🎉
During testing I noticed that the minimum dimension warning does not seem to take the mag into account. At least I had a bounding box with extent 32³, chose mag 2-2-2, and there was no warning that it was too small (although it is only 16³ vx in mag 2-2-2). I think this was wrong before your PR, so you don't need to fix it if it's more involved.
| @@ -153,7 +153,7 @@ class AiModelController @Inject()( | |||
| ) | |||
| existingAiModelsCount <- aiModelDAO.countByNameAndOrganization(request.body.name, | |||
| request.identity._organization) | |||
| _ <- bool2Fox(existingAiModelsCount == 0) ?~> "model.nameInUse" | |||
| _ <- bool2Fox(existingAiModelsCount == 0) ?~> "aiModel.nameInUse" | |||
| // it is necessary to save userBoundingBoxes separately and not load them from volumeTracings entries to support skeleton only annotations. | ||
| // Moreover, in case an annotations is a task, its task bounding box should also be used for training. |
There was a problem hiding this comment.
| // Moreover, in case an annotations is a task, its task bounding box should also be used for training. | |
| // Moreover, in case an annotation is a task, its task bounding box should also be used for training. |
| @@ -55,10 +56,12 @@ const FormItem = Form.Item; | |||
| // only the APIAnnotations of the given annotations to train on are loaded from the backend. | |||
| // Thus, the code needs to handle both HybridTracing | APIAnnotation where APIAnnotation is missing some information. | |||
| // Therefore, volumeTracings with the matching volumeTracingMags are needed to get more details on each volume annotation layer and its magnifications. | |||
| // The userBoundingBoxes are needed for checking for equal bounding box sizes. As training on fallback data is supported and an annotation is not required to have VolumeTracings, | |||
| // As the userBoundingBoxes should have multiple sizes of the smallest one, a check with a warning should be included. | |||
There was a problem hiding this comment.
| // As the userBoundingBoxes should have multiple sizes of the smallest one, a check with a warning should be included. | |
| // As the userBoundingBoxes should have extents that are multiples of the smallest extent, a check with a warning should be included. |
| `'${name}' of annotation ${annotationId}: ${boundingBox.join(", ")} -> ${alignedBoundingBox.join(", ")}`, | ||
| ); | ||
| warnings.push( | ||
| `The following bounding boxes are not aligned with the selected magnification. They will be automatically shrunk to be aligned with the magnification:\n${notMagAlignedBoundingBoxesStrings.join("\n")}`, |
There was a problem hiding this comment.
I think it would be helpful to include the actual magnification here, for example at the end of the first sentence.
There was a problem hiding this comment.
Yeah, the problem is that one can select different mags for different annotations. Therefore, I added the mag to the end of each bbox entry in the warning message.
But the problem is, that this makes the text even longer. I don't know if that's a benefit.
Maybe a better tree structure of the error message would help here 🤔. But this would also complicate the the code a little.
Here is what I currently have:
And this is what I mean with an improved hierarchy structure:
The following bounding boxes are not aligned with the selected magnification. They will be automatically shrunk to be aligned with the magnification:
- Annotation 677e8b3f5f0100a80b1dc6ae
- 'Bounding box 1' (3584, 3584, 1024, 64, 64, 64) will be 1792, 1792, 1024, 32, 32, 64 in mag 2, 2, 1
- 'Bounding box 2' (3648, 3648, 1088, 64, 64, 64) will be 1824, 1824, 1088, 32, 32, 64 in mag 2, 2, 1
- 'Bounding box 3' (3712, 3712, 1152, 64, 64, 64) will be 1856, 1856, 1152, 32, 32, 64 in mag 2, 2, 1 What do you think about this tree structured text?
There was a problem hiding this comment.
Ok, I made it work 🎉
Could you please check the newest changes?
- regarding bbox size calculation use bbox adjusted to mag (not just aligned)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (1)
678-691:⚠️ Potential issueImprove error handling in task validation.
The catch block silently swallows all errors to determine if the input is a task.
Apply this improvement:
- try { - const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl, { - showErrorToast: false, - }); - const finishedAnnotations = annotations.filter(({ state }) => state === "Finished"); - if (annotations.length > 0) { - annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id)); - } else { - unfinishedTasks.push(taskOrAnnotationIdOrUrl); - } - } catch (_e) { - isTask = false; - } + try { + const annotations = await getAnnotationsForTask(taskOrAnnotationIdOrUrl, { + showErrorToast: false, + }); + const finishedAnnotations = annotations.filter(({ state }) => state === "Finished"); + if (annotations.length > 0) { + annotationIdsForTraining.push(...finishedAnnotations.map(({ id }) => id)); + } else { + unfinishedTasks.push(taskOrAnnotationIdOrUrl); + } + } catch (error) { + if (error.response?.status === 404) { + isTask = false; + } else { + throw new Error(`Failed to fetch task ${taskOrAnnotationIdOrUrl}: ${error.message}`); + } + }
🧹 Nitpick comments (3)
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts (1)
209-211: Add JSDoc documentation for the new method.The method combines two operations (
alignWithMagandfromMag1ToMag) in a specific order. Adding documentation would help explain:
- The purpose of this method
- Why this specific order of operations is important
- What each strategy does to the bounding box
Add this documentation:
+ /** + * Aligns a bounding box with the given magnification and converts it from mag 1 to the target mag. + * This is particularly useful for training data where bounding boxes need to be properly aligned + * with the magnification to ensure consistent training samples. + * + * @param mag - The target magnification vector + * @param strategy - How to align the bounding box: + * - "shrink": ceil min and floor max + * - "grow": floor min and ceil max + * - "ceil": ceil both min and max + * - "floor": floor both min and max + * @returns A new aligned bounding box in the target magnification + */ alignFromMag1ToMag(mag: Vector3, strategy: "shrink" | "grow" | "ceil" | "floor"): BoundingBox {frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (2)
731-740: Consider using a distinct color for task bounding boxes.Currently using black ([0, 0, 0]) which might not be visually distinct enough.
Consider using a more distinctive color:
- color: [0, 0, 0], + color: [255, 140, 0], // Distinctive orange color for task bounding boxes
787-787: Improve the placeholder text for better clarity.The current placeholder "taskOrAnnotationIdOrUrl" could be more descriptive.
Consider this improvement:
- placeholder="taskOrAnnotationIdOrUrl" + placeholder="Enter task IDs, annotation IDs, or URLs (one per line)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/oxalis/model/bucket_data_handling/bounding_box.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/shader_editor.ts(1 hunks)frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/model/helpers/shader_editor.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx (2)
59-64: Well-documented changes!The comments clearly explain the requirements and considerations for:
- Bounding box size validation
- Task bounding box integration
- Support for fallback data
Line range hint
538-636: Good implementation of bounding box validation!The validation logic properly handles:
- Magnification alignment checks with clear warnings
- Minimum size requirements (32 voxels) as discussed
- Clear error messages with box details
There was a problem hiding this comment.
LGTM 👍
During testing I noticed that the minimum dimension warning does not seem to take the mag into account. At least I had a bounding box with extent 32³, chose mag 2-2-2, and there was no warning that it was too small (although it is only 16³ vx in mag 2-2-2). I think this was wrong before your PR, so you don't need to fix it if it's more involved.
Was this fixed by your changes or should it be fixed as a followup?
That should also be solved here. The screenshot shows that the mag adjusted size is used for minimal bounding box comparison |
|
And this screenshot includes the minimum warning message |
|
Awesome, thank you! |
This PR changes the AI Training Input under Administration -> AI Models to support task Ids. Additionally, the task bounding boxes of those tasks are now checked before submitting the training job. The corresponding worker pr ensures that the worker also uses the task bounding boxes during training.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)