Support segment lists in multi-annotation upload#9360
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements a staged merge for volume annotations: add initializeForMerge to persist emptied placeholder stats, store temporary MergedVolumeStats, then finalize merge in mergedFromContents. Adjust MergedVolumeStats shape, wire TemporaryMergedVolumeStatsStore, and preserve NML-derived tracing names. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
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: 1
🧹 Nitpick comments (1)
app/models/annotation/WKRemoteTracingStoreClient.scala (1)
210-225: Consider error handling for partial failures in the three-step merge.The three-step workflow (initializeForMerge → initialDataMultiple → mergedFromContents) has no explicit cleanup if an intermediate step fails. While the 1-hour expiry on
temporaryMergedVolumeStatsStoreprovides eventual cleanup, a failure afterinitializeForMergebut beforemergedFromContentsleaves an orphaned tracing at version 0 with empty segments.This may be acceptable given the rarity of this scenario and the eventual expiry, but consider whether explicit cleanup on failure would improve consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/MergedVolume.scala`:
- Around line 151-157: The stats method is passing
presentMags.map(vec3IntToProto) (Set[Vec3IntProto]) into
MergedVolumeStats.seenMags which expects Set[Vec3Int]; fix by passing the
original presentMags (no vec3IntToProto conversion) or alternatively change the
MergedVolumeStats.seenMags type to Set[Vec3IntProto] if the rest of the code
expects protos—update the stats method (symbol: stats) to use presentMags
directly or adjust the MergedVolumeStats definition accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22d3fb80-6de4-4a8b-9d47-effc52f23ffa
📒 Files selected for processing (11)
app/models/annotation/AnnotationUploadService.scalaapp/models/annotation/WKRemoteTracingStoreClient.scalaapp/models/annotation/nml/NmlResults.scalaunreleased_changes/9360.mdwebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TracingStoreModule.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/TemporaryTracingStore.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/MergedVolume.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeSegmentIndexService.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingService.scalawebknossos-tracingstore/conf/tracingstore.latest.routes
...racingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/MergedVolume.scala
Show resolved
Hide resolved
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
The code looks good, but I am a little unsure whether I can correctly follow the code changes.
So basically, the code was already able to merge multiple volume tracings and their data into one and also remap the segment ids via the mergedVolumeStats.idMaps, but it wasn't used before: The previous upload procedure did not do the 3rd additional step after merging the volume stats, and thus mergedVolumeStats.idMaps was disregarded and never really used, althougt the information was technically already present to calculate the correct segment stats list? So this PRs main changes are: Adding a 3rd step to use the ``mergedVolumeStats.idMaps` to be able to calculate the correct segment list? Or am I missing something 🤔
I'll do the testing tomorrow
|
Yes, that’s basically right. The MergedVolumeStats are a summary of the volume data merging process. It contains a map indicating the remapped ids. This remapping needs to be applied to the segment list as well. Since before this PR, the merging of volume data was the last step, and the tracing objects were merged first, the MergedVolumeStats were unavailable at that point (a mocked empty one was passed instead). Now, with the final step, all data is available and can be used to populate the protos. |
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
Ok here is what I did for testing till now:
-
I created 4 annotations on the same datasets.
-
Each got a new empty volume annotation layer
-
In each I selected 2 or 3 segment ids and drew with them a little. There were cell id duplicated between the annotations as well as unique onces per annotation-
-
Now I merged the 2nd into the 1st annotation via the menu in the navbar. In the modal I entered the annotation id. The merging succeeded, however: The data of the new volume layers without a fallback layer were all merged into the layer with the fallback data:
- annotation 1: segmentation + fallback & Volume
- annotation 2: segmentation + fallback & Volume
- merged annotation: segmentation + fallback (includes the annotations from the "Volume" layers)
-
I think this is a rather unwanted behaviour imo.
-
I did this with all 4 annotations pairwise so I ended up with a fully merged annotation
-
=> The segment list was correct 🎉 , but in the "wrong" layer.
-
When I tried the merging via nml / zip upload (I downloaded annotation 2 as zarr zip) the upload failed with the 2nd error message being "Could not merge volume annotations, as their magnifications differ. Please ensure each annotation has the same set of mags."
-
Then I tried to upload the wkw downloaded zip and this worked, but the result was incorrect imo and also did not add new segments and no separate group the the not added new segments was created. Not sure what went wrong there.
-
A second try also showed problem with the zip + wkw merge upload: The segment list was unchanged in the state before merging and only brush stroke from the annotation that was merged into was missing in the merged anntotation. Thus, I had the segment list entries from the 1st annotation, but not its data and the data from annotation 2 but not their segments :/.
Sorry, that this is so complicated and that I seem to find buggy behaviour although you already tested this. Maybe I am doing something wrong during testen? But, I just followed the workflow I'd say a user would come up with on their own 🤔
|
Thanks for testing so thoroughly! I think there is a little misunderstanding going on. I think the problems you described, though I did not understand each one fully, are related to merging annotations where each one has multiple volume layers. This case is indeed not yet properly supported, but that is not what this PR is about. I now wrote #9399 to track this case. This PR, however, is about drag’n’dropping multiple annotation zips (where each has just one volume layer) into the dashboard, creating a new annotation. So to test this, please create multiple annotations (where each has just one volume layer. Either all with fallback or none with fallback), download them, and then upload those zips together. Of course some of the described bugs could be happening there too, but I would assume they are independent. What you also tested was the import-volume-data case (Dragging an annotation zip into an existing annotation). This was also not touched in this PR. Looks like there is also a bug there. So I wrote #9401 to track that. I confirmed that both bugs are already present on master, so reviewing this PR can be independent of them. Sorry that my steps to take weren’t clearer! But it’s good to draw attention to these (somewhat unrelated) bugs too, of course :-) |
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
Awesome works 🎉
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/annotation/WKRemoteTracingStoreClient.scala`:
- Around line 211-225: The RPC calls to initializeForMerge and
mergedFromContents use the default timeout while initialDataMultiple uses
.withLongTimeout, causing potential timeouts on large merges; update the two rpc
invocations that call "${tracingStore.url}/tracings/volume/initializeForMerge"
and "${tracingStore.url}/tracings/volume/mergedFromContents" to also use
.withLongTimeout (same timeout behavior as the existing initialDataMultiple
call) so all three heavy merge-phase requests use a long timeout.
In
`@webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala`:
- Around line 211-217: The code currently calls
temporaryMergedVolumeStatsStore.pop(newTracingId) which destructively removes
the only MergedVolumeStats before calling volumeTracingService.merge and
saveVolume; change this to a non-destructive read (e.g.,
temporaryMergedVolumeStatsStore.get/read/peek) to obtain mergedVolumeStats, then
perform volumeTracingService.merge(tracingsFlat, mergedVolumeStats, ...) and
volumeTracingService.saveVolume(newTracingId, mergedTracing.version,
mergedTracing); only after saveVolume succeeds, delete the staged stats (call
pop/delete) for newTracingId, or if you prefer keep pop but wrap merge+save in a
try/finally that reinserts the mergedVolumeStats on failure so
mergedFromContents can be retried. Ensure you update references to
temporaryMergedVolumeStatsStore.pop(newTracingId) and the error path that
expects "mergedVolumeStats.notFound".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cf24277-566c-446b-b392-48bbc7d0de7a
📒 Files selected for processing (3)
app/models/annotation/WKRemoteTracingStoreClient.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scalawebknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/MergedVolume.scala
🚧 Files skipped from review as they are similar to previous changes (1)
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/MergedVolume.scala
...tore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingController.scala
Show resolved
Hide resolved
2 participants
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)