Implement option to export proofreading as segmentation #8286
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several enhancements to WEBKNOSSOS, focusing on job processing, annotation export, and user interface improvements. The changes primarily revolve around adding the ability to export proofreading as segmentation, updating job-related API calls, and refining bounding box handling. The modifications span multiple components, including backend controllers, frontend API interfaces, and job processing modals, with a particular emphasis on providing more flexible options for volume annotation jobs. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 0
🧹 Nitpick comments (7)
CHANGELOG.unreleased.md (1)
16-16: Enhance the changelog entry with more user-facing details.While the entry correctly documents the new feature, consider expanding it to include usage instructions and requirements. Based on the PR description, you could enhance it like this:
-Added the option to export proofreading as segmentation [#8286](https://github.com/scalableminds/webknossos/pull/8286) +Added the option to export proofreading as segmentation. Users can now materialize proofreading changes by selecting the three dots next to a segmentation layer in annotations with agglomerate file mapping. The changes will be reflected in the resulting Dataset. [#8286](https://github.com/scalableminds/webknossos/pull/8286)frontend/javascripts/admin/api/jobs.ts (1)
211-212: Consider adding type validation for the bounding box parameterThe implementation correctly handles the new parameters, but consider adding validation for the
selectedBoundingBoxparameter to ensure it's a valid Vector6 before converting to string.selectedBoundingBox?: Vector6, ): Promise<APIJob> { + if (selectedBoundingBox && selectedBoundingBox.length !== 6) { + throw new Error('Invalid bounding box: must be a Vector6'); + }Also applies to: 227-232
app/controllers/JobController.scala (1)
398-401: Consider adding size limit validation for the bounding boxWhile the code correctly validates the bounding box for non-super users, consider adding size limit validation to prevent processing extremely large volumes.
def validateBoundingBoxSize(bbox: String): Fox[Unit] = { for { parsed <- BoundingBox.fromLiteral(bbox).toFox _ <- bool2Fox(parsed.width * parsed.height * parsed.depth <= maxVolumeSize) ?~> "boundingBox.tooLarge" } yield () }frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (3)
894-894: Consider using useState for better state managementInstead of using a let variable, consider using React's useState hook for managing the includesProofreading state.
- let includesProofreading = false; + const [includesProofreading, setIncludesProofreading] = useState(false); const activeSegmentationTracingLayer = useSelector(getActiveSegmentationTracingLayer); // ... if (tracing.volumes.length === 0) { // ... } else { - includesProofreading = tracing.volumes.some((v) => v.hasEditableMapping === true); + useEffect(() => { + setIncludesProofreading(tracing.volumes.some((v) => v.hasEditableMapping === true)); + }, [tracing.volumes]); }Also applies to: 929-930
Line range hint
962-991: Consider adding type safety for the bounding box computationWhile the implementation is correct, consider adding type safety for the bounding box computation.
- const bbox = selectedBoundingBox?.boundingBox - ? computeArrayFromBoundingBox(selectedBoundingBox.boundingBox) - : undefined; + const bbox = selectedBoundingBox?.boundingBox + ? ((): Vector6 => { + const computed = computeArrayFromBoundingBox(selectedBoundingBox.boundingBox); + if (computed.length !== 6) { + throw new Error('Invalid bounding box: must be a Vector6'); + } + return computed as Vector6; + })() + : undefined;
Line range hint
894-991: Consider splitting the component for better maintainabilityThe MaterializeVolumeAnnotationModal component has multiple responsibilities. Consider:
- Extracting the bounding box logic into a separate hook
- Creating a separate component for the job configuration
- Adding proper error boundaries
This will improve maintainability and make the code easier to test.
conf/webknossos.latest.routes (1)
271-271: LGTM! Consider adding parameter documentation.The route change correctly implements the new parameters for proofreading export functionality. The parameter ordering follows good practices with required parameters first and optional ones last.
Consider adding comments to document:
- The format expected for
selectedBoundingBoxparameter- The impact of
includesProofreadingon the materialization process
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.unreleased.md(1 hunks)app/controllers/JobController.scala(3 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/api/jobs.ts(4 hunks)frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx(5 hunks)
🔇 Additional comments (4)
frontend/javascripts/admin/api/jobs.ts (1)
246-247: LGTM: Parameter propagation is correct
The new parameters are correctly propagated from startMaterializingVolumeAnnotationJob to startSegmentationAnnotationDependentJob.
Also applies to: 258-259
app/controllers/JobController.scala (2)
382-384: LGTM: Method signature changes are well-documented
The new parameters are properly added with appropriate types.
412-414: LGTM: Command arguments are properly updated
The new parameters are correctly added to the command arguments JSON object.
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (1)
960-960: LGTM: Bounding box configuration is correctly tied to proofreading state
The bounding box configuration is properly controlled by the includesProofreading flag.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/JobController.scala (1)
412-414: Handle optional parameters incommandArgsappropriatelyWhen adding
volumeLayerName,includesProofreading, andselectedBoundingBoxParsedtocommandArgs, ensure that optional parameters are correctly serialized. Directly insertingOptiontypes into a JSON object may result innullvalues, which might not be handled as expected downstream.Consider modifying the
commandArgsconstruction to include optional fields only when they are defined:val baseCommandArgs = Json.obj( "organization_id" -> organization._id, "dataset_name" -> dataset.name, "dataset_directory_name" -> dataset.directoryName, "fallback_layer_name" -> fallbackLayerName, "annotation_id" -> annotationId, "output_segmentation_layer_name" -> outputSegmentationLayerName, "annotation_type" -> annotationType, "new_dataset_name" -> newDatasetName, "merge_segments" -> mergeSegments, "includes_proofreading" -> includesProofreading ) val commandArgs = baseCommandArgs ++ volumeLayerName.map("volume_layer_name" -> Json.toJson(_)) ++ selectedBoundingBoxParsed.map("selected_bounding_box" -> Json.toJson(_))This approach adds optional fields to
commandArgsonly when they are defined, avoiding potentialnullvalues in the JSON object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/JobController.scala(3 hunks)
🔇 Additional comments (2)
app/controllers/JobController.scala (2)
398-401: Confirm proper handling of selectedBoundingBoxParsed
The code conditionally parses selectedBoundingBox based on includesProofreading and checks bounding box limits for non-super users. Ensure that selectedBoundingBoxParsed is correctly handled when includesProofreading is false to avoid potential issues with uninitialized variables or unintended behavior.
382-384: Ensure all calls to runMaterializeVolumeAnnotationJob are updated
The method signature of runMaterializeVolumeAnnotationJob has been updated to include includesProofreading and selectedBoundingBox parameters. Please verify that all invocations of this method elsewhere in the codebase have been updated to match the new signature to prevent runtime errors.
Run the following script to find all usages of runMaterializeVolumeAnnotationJob and check for proper parameter usage:
| @@ -13,6 +13,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released | |||
| ### Added | |||
| - Added the total volume of a dataset to a tooltip in the dataset info tab. [#8229](https://github.com/scalableminds/webknossos/pull/8229) | |||
| - Optimized performance of data loading with “fill value“ chunks. [#8271](https://github.com/scalableminds/webknossos/pull/8271) | |||
| - Added the option to export proofreading as segmentation [#8286](https://github.com/scalableminds/webknossos/pull/8286) | |||
There was a problem hiding this comment.
| - Added the option to export proofreading as segmentation [#8286](https://github.com/scalableminds/webknossos/pull/8286) | |
| - Added the option to export a segmentation that was corrected with the proofreading tool to a new segmentation. [#8286](https://github.com/scalableminds/webknossos/pull/8286) |
| @@ -925,6 +926,8 @@ export function MaterializeVolumeAnnotationModal({ | |||
| output dataset and the output segmentation layer. | |||
| </p> | |||
| ); | |||
| } else { | |||
| includesProofreading = tracing.volumes.some((v) => v.hasEditableMapping === true); | |||
There was a problem hiding this comment.
If my annotation contains two volume layers and only one has an editable mapping, that property will be true. However, i I want to export the layer that has no editable mapping, true will still be used in this UI and sent to the backend, which seems wrong?
| @@ -954,8 +957,13 @@ export function MaterializeVolumeAnnotationModal({ | |||
| jobName={"materialize_volume_annotation"} | |||
| suggestedDatasetSuffix="with_merged_segmentation" | |||
| chooseSegmentationLayer | |||
| isBoundingBoxConfigurable={includesProofreading} | |||
There was a problem hiding this comment.
only proofread layers support configuring the bbox? would it be easy to always support it when exporting volume annotations? no prio though.
| @@ -208,6 +208,8 @@ function startSegmentationAnnotationDependentJob( | |||
| annotationId: string, | |||
| annotationType: APIAnnotationType, | |||
| mergeSegments?: boolean, | |||
| includesProofreading?: boolean, | |||
| selectedBoundingBox?: Vector6, | |||
There was a problem hiding this comment.
"selected" sounds as if this variable would refer to a bounding box (ID) selected in the UI. I'd rename it to simply boundingBox.
| requestURL.searchParams.append("includesProofreading", includesProofreading.toString()); | ||
| } | ||
| if (selectedBoundingBox) { | ||
| requestURL.searchParams.append("selectedBoundingBox", selectedBoundingBox.toString()); |
There was a problem hiding this comment.
| requestURL.searchParams.append("selectedBoundingBox", selectedBoundingBox.toString()); | |
| requestURL.searchParams.append("selectedBoundingBox", selectedBoundingBox.join(",")); |
should be identical, but in your variant one has to know how JS serializes number arrays. I find it better to be explicit about it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/JobController.scala (1)
398-400: LGTM! Validation logic is secure and well-structured.The validation appropriately restricts non-super users from processing unbounded areas when proofreading is included. Consider extracting the condition to a named value for improved readability.
multiUser <- multiUserDAO.findOne(request.identity._multiUser) +val requiresBoundingBoxValidation = !multiUser.isSuperUser && includesProofreading -_ <- Fox.runIf(!multiUser.isSuperUser && includesProofreading)(Fox.runOptional(selectedBoundingBox)(bbox => +_ <- Fox.runIf(requiresBoundingBoxValidation)(Fox.runOptional(selectedBoundingBox)(bbox => jobService.assertBoundingBoxLimits(bbox, None)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/JobController.scala(3 hunks)
🔇 Additional comments (3)
app/controllers/JobController.scala (3)
382-384: LGTM! Method signature changes are well-structured.
The new parameters are appropriately typed and named, aligning with the PR's objective to support proofreading export functionality.
411-413: LGTM! Command arguments are properly structured.
The new fields are correctly added to the command arguments, maintaining consistency with the existing JSON structure and naming conventions.
Line range hint 382-413: Verify integration with job processing system.
Let's verify the usage of these new parameters across the codebase to ensure proper integration.
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about the integration:
Integration with job processing system is properly implemented.
The changes are well-integrated across the codebase:
- The job command
materialize_volume_annotationis properly registered inJobCommand.scala - Route definition in
webknossos.latest.routescorrectly includes all new parameters - Frontend components in
starting_job_modals.tsxhandle the new parameters - Proper error messages are defined in
conf/messages - Job processing infrastructure (workers) supports this command as shown in
dbtool.js - Security checks are in place for organization-level access and bounding box limits for non-super users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for references to materializeVolumeAnnotation job
echo "Checking for job references..."
rg -A 5 "materializeVolumeAnnotation|materialize_volume_annotation"
# Check for related test coverage
echo "Checking for related tests..."
fd -e scala -e java -e py test | xargs rg -l "materializeVolumeAnnotation|materialize_volume_annotation"
# Check for route definitions
echo "Checking route definitions..."
fd routes | xargs rg "materializeVolumeAnnotation"
Length of output: 12019
There was a problem hiding this comment.
I took the liberty to add a commit for the backend to make the linter happy, and remove a no-op line (no bbox was parsed there).
Backend LGTM now :)
Testing went well! 🎉
One thing: in the backend code, we usually speak of “editable mapping” rather than “proofreading”. @philippotto can you comment how this is handled in the frontend? Maybe we can unify the naming for the new job parameters as well.
yes, that's also how it's done in the front-end. |
|
Alternatively, it might make sense to call the parameter use_zarr_streaming. and set this one to true if the frontend has an editable mapping for this layer. This way we don’t have to adapt the worker if the logic will change eventually. Long-term, we might be able to switch more than just proofreading annotations to the zarr streaming 🤔 |
Yes, sounds good, too. |
philippotto
changed the title
Steps to test:
corresponding voxelytics pr