Export edited edges from editable mapping (proofreading annotation) #8816
Conversation
📝 WalkthroughWalkthroughThis change introduces the ability to export editable mapping (proofreading) annotations, including the edited edges, as part of annotation downloads. It adds logic to serialize edited mapping edges into a zipped Zarr3 format, updates relevant data models and controllers, and adds a new HTTP endpoint for retrieving the zipped edited edges. Supporting utilities and serialization methods are also updated or introduced. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fm3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/models/annotation/WKRemoteTracingStoreClient.scala (1)
283-290: Consider more specific error handling for JSON parsing.The JSON extraction at line 289 could fail silently if the
baseMappingNamefield is missing. Consider adding a more descriptive error message.- .flatMap(jsObj => JsonHelper.as[String](jsObj \ "baseMappingName").toFox) + .flatMap(jsObj => JsonHelper.as[String](jsObj \ "baseMappingName").toFox ?~> "editableMapping.baseMappingName.missing")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/controllers/AnnotationIOController.scala(1 hunks)app/models/annotation/WKRemoteTracingStoreClient.scala(5 hunks)app/models/annotation/nml/NmlWriter.scala(1 hunks)unreleased_changes/8816.md(1 hunks)util/src/main/scala/com/scalableminds/util/io/FileIO.scala(2 hunks)util/src/main/scala/com/scalableminds/util/tools/JsonHelper.scala(0 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/EmptyZarr3GroupHeader.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/Zarr3ArrayHeader.scala(3 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/annotation/AnnotationLayer.scala(3 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/EditableMappingController.scala(3 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingIOService.scala(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala(4 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/Zarr3BucketStreamSink.scala(1 hunks)webknossos-tracingstore/conf/tracingstore.latest.routes(1 hunks)
💤 Files with no reviewable changes (1)
- util/src/main/scala/com/scalableminds/util/tools/JsonHelper.scala
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: in webknossos scala codebase, when querying database tables with slick, explicit column listing in s...
Learnt from: frcroth
PR: scalableminds/webknossos#8821
File: app/models/dataset/Dataset.scala:864-866
Timestamp: 2025-08-04T11:49:30.012Z
Learning: In WebKnossos Scala codebase, when querying database tables with Slick, explicit column listing in SELECT statements is preferred over SELECT * to ensure columns are returned in the exact order expected by case class mappings. This prevents parsing failures when the physical column order in the production database doesn't match the schema definition order.
Applied to files:
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/Zarr3BucketStreamSink.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
📚 Learning: in the `updatemags` method of datasetmagsdao (scala), the code handles different dataset types disti...
Learnt from: frcroth
PR: scalableminds/webknossos#8609
File: app/models/dataset/Dataset.scala:753-775
Timestamp: 2025-05-12T13:07:29.637Z
Learning: In the `updateMags` method of DatasetMagsDAO (Scala), the code handles different dataset types distinctly:
1. Non-WKW datasets have `magsOpt` populated and use the first branch which includes axisOrder, channelIndex, and credentialId.
2. WKW datasets will have `wkwResolutionsOpt` populated and use the second branch which includes cubeLength.
3. The final branch is a fallback for legacy data.
This ensures appropriate fields are populated for each dataset type.
Applied to files:
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/Zarr3BucketStreamSink.scalawebknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/Zarr3ArrayHeader.scala
📚 Learning: in `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getmaginfo`, the use o...
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/model/sagas/proofread_saga.ts:1039-1039
Timestamp: 2024-11-22T17:18:04.217Z
Learning: In `frontend/javascripts/oxalis/model/sagas/proofread_saga.ts`, when calling `getMagInfo`, the use of `volumeTracingLayer.resolutions` is intentional and should not be changed to `volumeTracingLayer.mags`.
Applied to files:
app/models/annotation/nml/NmlWriter.scala
📚 Learning: in the webknossos codebase, classes extending `foximplicits` have access to an implicit conversion f...
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Applied to files:
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scalaapp/models/annotation/WKRemoteTracingStoreClient.scalautil/src/main/scala/com/scalableminds/util/io/FileIO.scala
📚 Learning: in scala's for-comprehension with fox (future-like type), the `<-` operator ensures sequential execu...
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Applied to files:
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scalaapp/models/annotation/WKRemoteTracingStoreClient.scalautil/src/main/scala/com/scalableminds/util/io/FileIO.scala
📚 Learning: in scala for-comprehensions with the fox error handling monad, `fox.frombool()` expressions should u...
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
Applied to files:
app/models/annotation/WKRemoteTracingStoreClient.scala
📚 Learning: in `dataseterrorloggingservice.scala`, prefer using `textutils.stacktraceasstring(exception)` when l...
Learnt from: frcroth
PR: scalableminds/webknossos#8202
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DatasetErrorLoggingService.scala:68-72
Timestamp: 2024-11-25T10:02:03.702Z
Learning: In `DatasetErrorLoggingService.scala`, prefer using `TextUtils.stackTraceAsString(exception)` when logging exceptions instead of passing the exception directly to `logger.error`.
Applied to files:
app/models/annotation/WKRemoteTracingStoreClient.scala
🔇 Additional comments (23)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/EmptyZarr3GroupHeader.scala (2)
5-8: LGTM! Well-structured Zarr3 group header implementation.The case class correctly defines the required Zarr3 group header fields with appropriate default values. The zarr_format field is correctly fixed to 3 for Zarr v3 compliance, and node_type is appropriately set to "group".
10-14: LGTM! Standard JSON serialization implementation.The companion object follows standard Play JSON patterns with the implicit formatter and provides a sensible filename constant for the Zarr JSON file.
util/src/main/scala/com/scalableminds/util/io/FileIO.scala (2)
8-8: LGTM! Appropriate JSON imports added.The Play JSON imports are correctly added to support the new JSON serialization functionality.
34-40: LGTM! Well-implemented JSON serialization utility.The
fromJsonSerializablemethod is well-designed with:
- Proper use of the Writes typeclass for type-safe JSON serialization
- Sensible default for pretty printing
- Clean delegation to existing
fromStringmethod- Good method signature with implicit parameters
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/annotation/AnnotationLayer.scala (3)
76-77: LGTM! Well-designed optional fields for edited mapping support.The new optional fields follow existing patterns and maintain backward compatibility:
editedMappingEdgesOptmirrors the existingvolumeDataOptpatternbaseMappingNameOptprovides necessary metadata- Both use
Optiontypes withNonedefaults for backward compatibility
87-91: LGTM! Consistent filename generation method.The
editedMappingEdgesZipNamemethod correctly follows the same pattern as the existingvolumeDataZipNamemethod, ensuring consistency in filename generation logic and handling of index/single scenarios.
98-112: LGTM! Factory method correctly updated.The
fromAnnotationLayermethod is properly updated to support the new optional parameters while maintaining backward compatibility through defaultNonevalues. The parameters are correctly passed through to the constructor.unreleased_changes/8816.md (1)
1-2: LGTM! Clear and informative changelog entry.The changelog entry accurately describes the new editable mapping download functionality and appropriately notes the current limitation that re-upload is not yet implemented. The mention of the zarr3-based format provides useful technical context.
webknossos-tracingstore/conf/tracingstore.latest.routes (1)
41-41: LGTM! Well-structured route addition.The new route is properly placed in the Editable Mappings section and follows established conventions:
- Uses appropriate GET method for download functionality
- Parameter types (String, Option[Long]) match expected controller signature
- Route path clearly indicates its purpose (
editedEdgesZip)app/controllers/AnnotationIOController.scala (1)
505-509: LGTM! Clean integration of edited mapping edges.The code correctly follows the existing pattern for adding volume data to the ZIP archive. Setting the compression level to
BEST_SPEEDis appropriate for maintaining consistency with the volume data handling above.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/Zarr3BucketStreamSink.scala (2)
63-63: LGTM! Good refactoring to centralize JSON serialization.Using
fromJsonSerializableinstead of manual JSON serialization improves code maintainability and consistency across the codebase.
65-67: LGTM! Consistent application of the serialization refactoring.The change maintains the same functionality while using the centralized JSON serialization approach.
app/models/annotation/nml/NmlWriter.scala (2)
283-283: LGTM! Updated to use the correct accessor method.The change from
hasEditableMapping.getOrElse(false)togetHasEditableMappingaligns with the protobuf accessor pattern and is more concise.
291-296: LGTM! Clean integration of edited mapping edges metadata.The conditional writing of
editedMappingEdgesLocationandeditedMappingBaseMappingNameattributes follows the established pattern and correctly integrates the new editable mapping features into the NML export format.webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/Zarr3ArrayHeader.scala (3)
31-31: LGTM! Clear documentation of boolean handling limitations.The updated comment clearly explains the constraints around boolean dataset handling, which is helpful for future developers.
239-241: LGTM! Correct boolean fill_value handling.The logic correctly handles boolean data types by converting the string representation ("true") to a proper JSON boolean value. This ensures proper zarr3 format compliance.
259-263: LGTM! Improved JSON object construction.The conditional inclusion of optional fields (
storage_transformersanddimension_names) is a good practice that avoids polluting the JSON with undefined fields while maintaining clean, readable code.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/EditableMappingController.scala (3)
13-13: LGTM! Appropriate import for the new functionality.The import correctly adds the
EditableMappingIOServiceneeded for the new edited edges export feature.
30-31: LGTM! Proper dependency injection setup.The dependency injection for
EditableMappingIOServicefollows the established pattern in the controller constructor.
150-168: LGTM! Well-implemented controller action for edited edges export.The
editedEdgesZipmethod correctly follows established patterns in the controller:
- Proper access token validation
- Standard error handling with Fox
- Logical flow: validate access → fetch data → process → return response
- Appropriate use of the injected
EditableMappingIOServiceThe implementation looks solid and integrates well with the existing codebase.
app/models/annotation/WKRemoteTracingStoreClient.scala (1)
277-282: LGTM! Good use of conditional data fetching.The implementation correctly fetches edited mapping edges only when the tracing has editable mapping, avoiding unnecessary network calls.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingIOService.scala (1)
39-103: Well-structured zarr3 serialization implementation.The implementation correctly handles:
- Chunking for memory efficiency
- Proper zarr3 header construction with appropriate metadata
- Efficient compression using Blosc
- Clear separation of concerns between edge data and boolean flags
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
540-575: Well-implemented edge extraction logic.The implementation correctly:
- Fetches and processes update groups with proper version handling
- Uses
ironOutRevertsto handle reverted actions appropriately- Distinguishes between split (false) and merge (true) operations
- Filters actions by the specific tracing ID
- Handles optional segment IDs with position fallback
...calableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingIOService.scala
Show resolved
Hide resolved
...calableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingIOService.scala
Show resolved
Hide resolved
...calableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingIOService.scala
Show resolved
Hide resolved
fm3
changed the title
There was a problem hiding this comment.
Awesome thanks for this new feature.
I left a few minor comments below.
Regarding testing:
- When downloading as wkw, the edited edges are still exported as zarr. This is a little unexpected but I'd say fine for now. Alternatively, wkw download could omit the edited edges
Besides that, everything is awesome :)
Side note: To open the zarr array I needed to do smth like this. Not sure why though:
tensorstore.open({
'driver': 'zarr3',
'kvstore': {
'driver': 'file',
'path': '<path>/editedMappingEdges_Volume/edges'
}
}).result().read().result()
...atastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/Zarr3ArrayHeader.scala
Show resolved
Hide resolved
.../scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala
Show resolved
Hide resolved
...re/app/com/scalableminds/webknossos/tracingstore/tracings/volume/Zarr3BucketStreamSink.scala
Show resolved
Hide resolved
...calableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingIOService.scala
Outdated
Show resolved
Hide resolved
| private lazy val compressorConfiguration = | ||
| BloscCodecConfiguration( | ||
| BloscCompressor.defaultCname, | ||
| BloscCompressor.defaultCLevel, | ||
| StringCompressionSetting(BloscCodecConfiguration.shuffleSettingFromInt(BloscCompressor.defaultShuffle)), | ||
| Some(BloscCompressor.defaultTypesize), | ||
| BloscCompressor.defaultBlocksize | ||
| ) | ||
|
|
||
| private lazy val compressor = | ||
| new BloscCompressor( | ||
| Map( | ||
| BloscCompressor.keyCname -> StringCompressionSetting(BloscCompressor.defaultCname), | ||
| BloscCompressor.keyClevel -> IntCompressionSetting(BloscCompressor.defaultCLevel), | ||
| BloscCompressor.keyShuffle -> IntCompressionSetting(BloscCompressor.defaultShuffle), | ||
| BloscCompressor.keyBlocksize -> IntCompressionSetting(BloscCompressor.defaultBlocksize), | ||
| BloscCompressor.keyTypesize -> IntCompressionSetting(BloscCompressor.defaultTypesize) | ||
| )) |
There was a problem hiding this comment.
So the configs in compressorConfiguration and compressor always need to be kept in sync? Isn't is possible to just create the compressor and get its configuration? We coded this ourselves. So this might be something for a future refactoring 🤔?
There was a problem hiding this comment.
Good point, I wrote #8842 for that
It’s always empty in this case anyway, so I’d say it makes more sense to omit it. Also let’s interpret skipVolumeData so that the proofreading zip is also omitted Follow-up for #8816
2 participants
Editable mapping (aka supervoxel proofreading) annotations can now be downloaded and the edited edges are included in a zarr3-based format. Note that re-upload is not yet implemented.
Format description:
<volume>tag in the NML now mentions theeditedMappingBaseMappingNamefor editableMapping annotations.editedMappingEdgesLocation, which points to a zipfileSteps to test:
tensorstore.open("file:///myAnnotation/editedMappingEdges_Volume/edgeIsAddition").result().read().result()and check that the result looks correct (ordering of the two arrays should match and be in editing order)TODOs:
Issues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)