WIP: Unified annotation versioning

[fm3]

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• abc

TODOs:

• [x] Mechanism to Revert Editable Mappings
  • [x] Iterators for SegmentToAgglomerate, AgglomerateToGraph
  • [x] How to encode Reverted chunks, agglomerates? → Single Zero-Byte?
    • [x] Check a single zero byte is not a valid proto message
  • [x] Iterate over current version, fetch old version, rewrite
  • [x] Integrate this in Updater or flush Updater before this happens
  • [x] Test
  • [ ] Cleanup: Generic Reversion-Aware Iterator? Build on top of VersionedFossilDbIterator?
  • [ ] Save perf by skipping fetching content when only version is needed in iterator (different from volume, because here we don’t need to update the segment index)
    • that might need new fossildb api (ListKeysWithVersions)
  • [ ] Make the iterator async?
• [x] Annotation proto object
• [ ] Design Annotation-wide update actions
  • [x] add layer
    • [x] becomes update action rather than route
    • [x] after applying updates, a summary is sent to wk if postgres-cached properties change
      • [x] could this lead to annotationId lookups before postgres knows about the new layer?
    • [x] assert no duplicate names?
    • [x] assert no more than one skeleton?
    • [x] test add layer as very first update action
  • [x] delete layer
  • [x] update layer metadata
  • [x] update annotation metadata (name+description)
  • [ ] iron out reversion folds + layer deletions in merge + duplicate?
• [x] Test sandbox annotation
• [x] Adapt Task creation (save annotationProto object)
• [ ] Duplicate
  • [x] Use in duplicate route (“copy to my account”)
  • [x] Use in task creation from base annotation
  • [x] Use in task assignment
  • [ ] What to do with task resetToBase? implement as revert action?
  • [ ] actionTracingIds need to be remapped in duplicate (or use layer names after all?)
  • [ ] duplicate history?
    • [x] duplicate update actions (needed for merging editable mappings)
    • [ ] duplicate v0 in addition to current version?
    • [ ] Also in fromTask case? How to mark earliest accessible version? We don’t want users to revert too far, right?
    • [ ] Should we also copy intermediate materialized versions? Or just 0 and current?
    • [ ] What about intermediate bucket versions? They are not in the updates
    • [ ] perf: duplicate api for fossilDB?
• [ ] Unified versioning over layers
  • [x] Route
  • [x] Store Updates
  • [x] Create Annotation
  • [x] Updates need Layer Identifier
  • [ ] Apply Updates
    • [x] updates that mutate annotation object
    • [x] updates that mutate tracing objects
    • [ ] set individual targetVersions for updater/buffers? or is this already done?
    • [x] updates that mutate other stuff
      • [x] volume buckets
      • [x] proofreading
    • [ ] Special updates
      • [x] AddSegmentIndex → functionality removed. only sets bool now.
      • [x] ImportVolumeData
      • [x] makeMappingEditable
      • [ ] Merge into current? → does not exist, only ImportVolumeData
      • [ ] Downsample? (Maybe remove feature)
      • [x] RevertToVersion
        • [x] Revert volume buckets
        • [x] Revert skeletons
        • [x] Revert annotation-level properties
        • [x] Revert proofreading fields
        • [x] Handle gone layers (either in previous or target version)
        • [x] What if two of those come in the same update batch?
        • [x] split updates batches by RevertToVersion? What are the intermediate versions?
  • [ ] Replace or fix MergedFromIds (used for compound, mergeTwo, always creates new annotation)
    • [x] merge skeletons
    • [x] merge volume data
    • [ ] merge history?
    • [ ] merge editable mappings
    • [ ] use persist bool in all cases or assert non-supported don’t happen
    • [ ] test with compound
  • [ ] Replace or fix MergedFromContents (only used during upload, so everything has v0)
  • [x] report applied updates to postgres only when requesting newest
  • [x] Version assertions
  • [x] lazy apply for volumes and editable mappings?
    • [x] can volume data still be written directly? can there be conflicts?
    • [ ] or ditch lazy apply completely? (might be ok with distributed skeletons etc)
    • [ ] When to materialize which layer?
      • [ ] store materialized layer only sometimes? count its update actions?
  • [ ] Ensure no parallel update applying on the same object (async cache?)
    • [x] But parallel update applying should also not be a problem, except for perf (was it only a problem because version wasn’t specified when loading volume buckets during revert?)
    • [ ] Perf: Reduce cache mem overhead by removing older versions when newer are requested (could be done by nested LRU cache, one with small capacity per annotationId)
    • [ ] Perf: When newer is requested, get old one from cache, then apply updates?
  • [ ]
• [ ] Search for // TODO
• [ ] Frontend
  • [x] Linearized update actions
  • [x] Use new update action route
  • [x] update actions now need actionTracingId
  • [ ] importVolumeData seems to send outdated version, does not reload on success?
  • [x] Some update actions now need to be distinguished between skeleton & volume, thus they are now separate and need their own updateName. This goes for:
    • updateUserBoundingBoxes → updateUserBoundingBoxesInSkeletonTracing, updateUserBoundingBoxesInVolumeTracing
    • ~~updateUserBoundingBoxVisibility → updateUserBoundingBoxVisibilityInSkeletonTracing,
    • updateUserBoundingBoxVisibilityInVolumeTracing~~ is used nowhere in the frontend -> remove in backend?
    • updateTracing → updateVolumeTracing, updateSkeletonTracing
  • [x] Some routes are now update actions (add layer, delete layer). makeHybrid was removed.
    • They need to first send an update action to the store and then e.g. reload
  • [x] Version Restore View; See: https://github.com/scalableminds/webknossos/pull/7917#issuecomment-2413488436
    • [x] For volume actions show which volume layer was edited (if the layer was deleted, show "unknown layer" / layer id)
    • [ ] Seems to break if layer set has changed (e.g. if a layer was added since the old version)
  • [ ] enforce revert actions to come in separate update group (has its own version number)
  • [ ] same for add layer / delete layer / initialize editable mapping
  • [ ] why is newestVersion sometimes called with emptystring annotationId?
  • [ ] Load the annotation proto object to get correct layer set, name, description for requested version (postgres only knows latest for the dashboard)
• [ ] Migration
  • [ ] Linearized Updates
  • [ ] changed update actions
    • [ ] several actions were renamed, e.g. updateTracing was renamed → updateVolumeTracing/updateSkeletonTracing
    • [ ] update actions now need actionTracingId
  • [ ] Find annotation ids
  • [ ] Can we run a first part in the background?
    • [ ] on a second run, (re)do only annotations that in postgres have a modified date newer than when first run started
  • [ ] editable mapping update actions and arrays used to be stored by mappingName, now annotationId
  • [ ] while we’re migrating everything, could we also do #3546 ?
    • [ ] how does it interact with ND data?
• [ ] migration guide
  • [ ] addSegmentIndex is removed. if you want to add segment indices to existing annotations, update to a lower version first, run the migration route, then upgrade to this
  • [ ] need to run fossildb migration
• [ ] changelog
  • [ ] removed downsample button (?)
  • [ ] unified annotation versioning

Issues:

• fixes #7839
• fixes #8029
• fixes #7703

---

(Please delete unneeded items, merge only when none are left open)

• [ ] Updated changelog
• [ ] Updated migration guide if applicable
• [ ] Updated documentation if applicable
• [ ] Adapted wk-libs python client if relevant API parts change
• [ ] Removed dev-only changes like prints and application.conf edits
• [ ] Considered common edge cases
• [ ] Needs datastore update after deployment

[fm3]

@philippotto A first rudimentary version seems to work: I can create an annotation on l4_sample, (note: volume buckets won’t load), take the annotation id and skeleton tracing id, and insert them in here (both in uri and in request body):

await fetch("http://localhost:9000/tracings/annotation/66cf0fc1d10000f6ae2f30bd/update?token=secretSampleUserToken", {
    "credentials": "include",
    "headers": {
        "Accept": "application/json",
        "Accept-Language": "en-US,en;q=0.5",
        "content-type": "application/json",
        "Sec-Fetch-Dest": "empty",
        "Sec-Fetch-Mode": "cors",
        "Sec-Fetch-Site": "same-origin",
        "Priority": "u=0",
        "Pragma": "no-cache",
        "Cache-Control": "no-cache"
    },
    "referrer": "http://localhost:9000/annotations/66cedcb2e000008402396d82",
    "body": "[{\"version\":1,\"transactionId\":\"lfh7mxycvn\",\"transactionGroupCount\":1,\"transactionGroupIndex\":0,\"timestamp\":1724832956301,\"authorId\":\"66c46740d10000d1005405b6\",\"actions\":[{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"createTree\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"id\":1,\"color\":[0.6784313725490196,0.1411764705882353,0.050980392156862744],\"name\":\"explorative_2024-08-28_Sample_User_001\",\"timestamp\":1724832956293,\"comments\":[],\"branchPoints\":[],\"groupId\":null,\"isVisible\":true,\"type\":\"DEFAULT\",\"edgesAreVisible\":true}},{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"createNode\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"additionalCoordinates\":[],\"radius\":1,\"rotation\":[0,0,0],\"viewport\":0,\"resolution\":0,\"id\":1,\"timestamp\":1724832956293,\"bitDepth\":8,\"interpolation\":false,\"position\":[3530,3603,1024],\"treeId\":1}},{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"updateSkeletonTracing\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"activeNode\":1,\"editPosition\":[3584,3584,1024],\"editPositionAdditionalCoordinates\":[],\"editRotation\":[0,0,0],\"zoomLevel\":1}},{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"updateTdCameraSkeleton\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\"}}],\"stats\":{\"treeCount\":1,\"nodeCount\":1,\"edgeCount\":0,\"branchPointCount\":0},\"info\":\"[\\\"UPDATE_LAYER_SETTING\\\",\\\"SET_HISTOGRAM_DATA_FOR_LAYER\\\",\\\"UPDATE_MESH_FILE_LIST\\\",\\\"UPDATE_CURRENT_MESH_FILE\\\",\\\"SET_INPUT_CATCHER_RECTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"SET_STORED_LAYOUTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING * 2\\\",\\\"SET_TOOL\\\",\\\"CREATE_NODE\\\"]\"},{\"version\":2,\"transactionId\":\"bmwafojnac\",\"transactionGroupCount\":1,\"transactionGroupIndex\":0,\"timestamp\":1724832956421,\"authorId\":\"66c46740d10000d1005405b6\",\"actions\":[{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"updateSkeletonTracing\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"activeNode\":1,\"editPosition\":[3552,3595,1024],\"editPositionAdditionalCoordinates\":[],\"editRotation\":[0,0,0],\"zoomLevel\":1}},{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"updateTdCameraSkeleton\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\"}}],\"stats\":{\"treeCount\":1,\"nodeCount\":1,\"edgeCount\":0,\"branchPointCount\":0},\"info\":\"[\\\"UPDATE_MESH_FILE_LIST\\\",\\\"UPDATE_CURRENT_MESH_FILE\\\",\\\"SET_INPUT_CATCHER_RECTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"SET_STORED_LAYOUTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING * 2\\\",\\\"SET_TOOL\\\",\\\"CREATE_NODE\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"MOVE_TD_VIEW_BY_VECTOR_WITHOUT_TIME_TRACKING\\\"]\"},{\"version\":3,\"transactionId\":\"anjed8c2bb\",\"transactionGroupCount\":1,\"transactionGroupIndex\":0,\"timestamp\":1724832956939,\"authorId\":\"66c46740d10000d1005405b6\",\"actions\":[{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"createNode\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"additionalCoordinates\":[],\"radius\":1,\"rotation\":[0,0,0],\"viewport\":0,\"resolution\":0,\"id\":2,\"timestamp\":1724832956939,\"bitDepth\":8,\"interpolation\":false,\"position\":[3514,3571,1024],\"treeId\":1}},{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"createEdge\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"treeId\":1,\"source\":1,\"target\":2}},{\"name\":\"updateSkeletonTracing\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"activeNode\":2,\"editPosition\":[3530,3603,1024],\"editPositionAdditionalCoordinates\":[],\"editRotation\":[0,0,0],\"zoomLevel\":1}}],\"stats\":{\"treeCount\":1,\"nodeCount\":2,\"edgeCount\":1,\"branchPointCount\":0},\"info\":\"[\\\"UPDATE_CURRENT_MESH_FILE\\\",\\\"SET_INPUT_CATCHER_RECTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"SET_STORED_LAYOUTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING * 2\\\",\\\"SET_TOOL\\\",\\\"CREATE_NODE\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"MOVE_TD_VIEW_BY_VECTOR_WITHOUT_TIME_TRACKING\\\",\\\"CREATE_NODE\\\"]\"},{\"version\":4,\"transactionId\":\"mgb0fqyu0c\",\"transactionGroupCount\":1,\"transactionGroupIndex\":0,\"timestamp\":1724832957039,\"authorId\":\"66c46740d10000d1005405b6\",\"actions\":[{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"updateSkeletonTracing\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"activeNode\":2,\"editPosition\":[3522,3587,1024],\"editPositionAdditionalCoordinates\":[],\"editRotation\":[0,0,0],\"zoomLevel\":1}},{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"updateTdCameraSkeleton\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\"}}],\"stats\":{\"treeCount\":1,\"nodeCount\":2,\"edgeCount\":1,\"branchPointCount\":0},\"info\":\"[\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"SET_STORED_LAYOUTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING * 2\\\",\\\"SET_TOOL\\\",\\\"CREATE_NODE\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"MOVE_TD_VIEW_BY_VECTOR_WITHOUT_TIME_TRACKING\\\",\\\"CREATE_NODE\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"MOVE_TD_VIEW_BY_VECTOR_WITHOUT_TIME_TRACKING\\\"]\"},{\"version\":5,\"transactionId\":\"9uu9b6j1qf\",\"transactionGroupCount\":1,\"transactionGroupIndex\":0,\"timestamp\":1724832957149,\"authorId\":\"66c46740d10000d1005405b6\",\"actions\":[{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"name\":\"updateSkeletonTracing\",\"value\":{\"actionTracingId\":\"3c2710c5-3b68-4bfa-ba7c-86553df9d8bf\",\"activeNode\":2,\"editPosition\":[3514,3571,1024],\"editPositionAdditionalCoordinates\":[],\"editRotation\":[0,0,0],\"zoomLevel\":1}}],\"stats\":null,\"info\":\"[\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"SET_STORED_LAYOUTS\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING * 2\\\",\\\"SET_TOOL\\\",\\\"CREATE_NODE\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"MOVE_TD_VIEW_BY_VECTOR_WITHOUT_TIME_TRACKING\\\",\\\"CREATE_NODE\\\",\\\"SET_TD_CAMERA_WITHOUT_TIME_TRACKING\\\",\\\"MOVE_TD_VIEW_BY_VECTOR_WITHOUT_TIME_TRACKING\\\"]\"}]",
    "method": "POST",
    "mode": "cors"
});

Then reload and see two skeleton nodes. Adding/Removing layer, volume annotation, proofreading is all still broken. So are the version assertions. But this can be used as a starting point to adapt the frontend:

• [X] include actionTracingIds in update actions
• [X] send updates to new endpoint
• [x] some update action names have changed (need to distinguish btw skeleton + volume). This may change again, though…
• [x] ~adapt tracingstore js client to include annotation id in everything (note: some backend-internal routes still have to be adapted too)~ I ended up looking up the annotation id by the tracingid instead.

Note: when switching to and from this branch, I clear the fossildb content with rm -rf fossildb/data; git checkout fossildb/data/.gitignore, otherwise the column family change is rejected

[fm3]

The next part of the frontend could be added now: The Restore Older Version flow. The version list view should no longer feature tabs per layer, but instead only one global GET tracings/annotation/:annotationId/updateActionLog route exists with a single list. (I currently adapted the frotnend so that each tab uses this new route)

Also, the revertToVersion update action is no longer layer specific (I guess this may already be covered by philipps changes).

It is required that the revertToVersion update action is the only action in its update group (so that it gets its own version number). Not sure if the frontend already does that.

Since Philipp is out of office, maybe @MichaelBuessemeyer could have a look at this after #6613 is done?

Note that this branch opens fossildb with different column families. I run rm -rf fossildb/data; git co fossildb/data/.gitignore when switching to and from this branch to avoid errors. If you need your fossildb data, you can of course create a backup first.

[MichaelBuessemeyer]

Since Philipp is out of office, maybe @MichaelBuessemeyer could have a look at this after https://github.com/scalableminds/webknossos/issues/6613 is done?

Sure :)

[fm3]

@MichaelBuessemeyer the next batch of frontend todos has arrived :innocent: I have not turned all of them into checkboxes in the description, feel free to do that if you prefer.

• Some update actions are now duplicated, because I have to distinguish between SkeletonUpdateActions and VolumeUpdateActions, even if they are basically identical:

  • updateUserBoundingBoxes → updateUserBoundingBoxesInSkeletonTracing, updateUserBoundingBoxesInVolumeTracing
  • updateUserBoundingBoxVisibility → updateUserBoundingBoxVisibilityInSkeletonTracing, updateUserBoundingBoxVisibilityInVolumeTracing
  • updateTracing → updateVolumeTracing, updateSkeletonTracing

• no renaming, but note: these is an AnnotationUpdateAction (no actionTracingId)

  • updateTdCamera
  • revertToVersion

• New update actions that were previously separate routes, either to wk or to tracingstore:

  • addLayerToAnnotation
    • layerParameters: AnnotationLayerParameters (same as before: typ, name, magRestrictions, etc)
  • deleteLayerFromAnnotation
    • tracingId: String
    • layerName: String
    • type: AnnotationLayerType (enum, "Skeleton", "Volume")
  • updateLayerMetadata
    • tracingId: String
    • layerName: String
  • updateMetadataOfAnnotation
    • name: Option[String]
    • description: Option[String]

• The frontend should fetch the new annotation proto object from GET {tracingstore_uri}/tracings/annotation/:annotationId to get correct layer set, name, description for requested version (postgres only knows latest for the dashboard). This is especially relevant during version restore view (or if we eventually add permalinks to open previous versions).

[MichaelBuessemeyer]

Regarding the first point:

Some update actions are now duplicated, because I have to distinguish between SkeletonUpdateActions and VolumeUpdateActions, even if they are basically identical:

It seems like @philippotto already implemented this :partying_face:

But regarding:

updateUserBoundingBoxVisibility → updateUserBoundingBoxVisibilityInSkeletonTracing, updateUserBoundingBoxVisibilityInVolumeTracing

The frontend does not know this update action. Maybe it was deprecated and is no longer sent by the frontend? At least cannot find it in the frontend code of this branch and the master as well :woozy_face:. Maybe this is an old update action that is still supported by the backend for compatibility reasons? But on the other side, the version restore view does not have such an entry for legacy purposes. A quick check of the backend master code shows, that updateUserBoundingBoxVisibility indeed exists but I don't see any place in the frontend that would send such an update action. Moreover, I just checked what happens if the visibility of one of the user bboxes is changed. The frontend sends an updateAction in form of a "updateUserBoundingBoxes".

=> Maybe the updateUserBoundingBoxVisibility updateAction can be removed from the backend code :thinking:?

[coderabbitai[bot]]

[!IMPORTANT]

Review skipped

More than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review.

116 files out of 230 files are above the max files limit of 75. Please upgrade to Pro plan to get higher limits.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[MichaelBuessemeyer]

@fm3 While implementing the new update actions which replaced some routes I came across the following questions:

deleteLayerFromAnnotation
tracingId: String
layerName: String
type: AnnotationLayerType (enum, "Skeleton", "Volume")

Why is the layerName and type needed for this annotation action? The tracingId should be unique enough to identify the layer, isn't it?

Answer: This is only used for more detailed information in the version restore view.

For update action

updateMetadataOfAnnotation
name: Option[String]
description: Option[String]

the previous equivalent route was able to update the following annotation properties:

export type EditableAnnotation = {
  name: string;
  description: string;
  visibility: APIAnnotationVisibility;
  tags: Array<string>;
  viewConfiguration?: AnnotationViewConfiguration;
};

export function editAnnotation(
  annotationId: string,
  annotationType: APIAnnotationType,
  data: Partial<EditableAnnotation>,
): Promise<void> {
  return Request.sendJSONReceiveJSON(`/api/annotations/${annotationType}/${annotationId}/edit`, {
    data,
    method: "PATCH",
  });
}

Meaning the route was not only able to update the description and name but also the tags, visibility and viewConfig. Could this also be added to the updateMetadataOfAnnotation update action? If not, another update action is needed to update the missing three properties. At least that's how I see / understand it right now :)

Answer: The name and description will be versioned as update actions but the editAnnotation route will still exist and take updates regarding tags, visibility, and viewConfig.

[MichaelBuessemeyer]

Some more questions :D

The frontend should fetch the new annotation proto object from GET {tracingstore_uri}/tracings/annotation/:annotationId to get correct layer set, name, description for requested version (postgres only knows latest for the dashboard). This is especially relevant during version restore view (or if we eventually add permalinks to open previous versions).

What about sandbox annotations? The current route for this is: /api/datasets/${datasetId.owningOrganization}/${datasetId.name}/sandbox/${tracingType}

Answer: Keep it. & Sandbox is only supported for skeleton annotations.

Moreover, the old annotation info fetching route (/api/annotations/${annotationId}/info?timestamp=${Date.now()};`) is still required as the frontend needs to identify the tracing store URL for the annotation.

TODO [ ]: Always only use the info from the TS route. Maybe the core backend has layers that were deleted already and so on.

TODO: micha [ ] Test me with version restore

Moreover, is it possible to split up an annotation (its layers) across multiple tracing stores? If that's the case, the frontend needs to call all tracing stores available in the maybe outdated annotation info object from the backend and then merge this information?

There can be only one TS at a time for a single WK instance. => So this is not possible

[fm3]

note to self: note to self, open questions:

• remove downsample feature? → yes
• duplicate history?
  • duplicate update actions (needed for merging editable mappings)
  • duplicate v0 in addition to current version?
  • Also in task assignment case? How to mark earliest accessible version? We don’t want users to revert too far, right?
  • Should we also copy intermediate materialized versions? Or just 0 and current?
  • What about intermediate bucket versions? They are not in the updates
  • perf: duplicate api for fossilDB?
• resetToBase as update action?
• during apply, what to load in memory? is it all or nothing? → load all, as usually we request everything
• during apply, what to flush to fossil? is it all or nothing? → flush all layers that were changed (updates for the layer exist)
• avoid duplicate update applying (async cache?)

[MichaelBuessemeyer]

Making an annotation's mapping editable (starting proofreading actions should work again.

Although, I'd like to ask why the updateMappingName actions now makes a mapping editable. To me this sounds kinda like magic because I would not have expected an action called updateMappingName to make a mapping editable. Can we maybe make this more obvious? E.g. an extra action or so? or at least a parameter / property of the updateMappingName like makeMappingEditable or isMappingEditable?

[fm3]

or at least a parameter / property of the updateMappingName like makeMappingEditable or isMappingEditable?

Yes, such an extra property is required! It’s called isEditable. For this particular context, both isEditable and isLocked should be set to true. https://github.com/scalableminds/webknossos/blob/master/webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala#L359

Thanks for having a look at this!

[MichaelBuessemeyer]

Yes, such an extra property is required! It’s called isEditable. For this particular context, both isEditable and isLocked should be set to true. https://github.com/scalableminds/webknossos/blob/master/webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeUpdateActions.scala#L359

Thanks for having a look at this!

Oh right, thanks. I got confused between updateActions and dispatchActions here, because the dispatch action does not have fields like isLocked and isEditable. But it should work now :tm:

[fm3]

@MichaelBuessemeyer the backend should be ready for review. I’ll continue by building the FossilDB migration, this will occupy me for some time. I’d suggest to first finish the frontend here as far as possible (maybe @philippotto can help with that once he’s back) and then you could start the backend review here. I’ll happily give an introduction in person.

[fm3]

You’re right, I’ll add that! I didn’t even remember that the time tracking api includes the stats :sweat_smile:

[MichaelBuessemeyer]

@philippotto feel free to ping me in case you have problems with merge conflicts which might be created by #8075

[fm3]

@normanrz the migration should be ready for a first review round. Could you please have a look at the code in tools/migration-unified-annotation-versioning?

[MichaelBuessemeyer]

re-requesting review for frontend

[MichaelBuessemeyer]

MichaelBuessemeyer requested a review from philippotto now

sorry, misclick :see_no_evil:

[fm3]

Thanks a lot for the thorough review @MichaelBuessemeyer ! You definitely found some bugs that would have caused headaches later! I tried to answer your questions and address the feedback. I was unsure about some things, maybe we can discuss them further. I replied to most threads.

Concerning the changes in moved files, of course I’ll try to keep the commits separate next time! I hope I’ll think of it in time. Sorry about the inconvenience!

[MichaelBuessemeyer]

The newest frontend changes also look good to me :)

[fm3]

Thanks for your testing!

Is it correct, that t in case an annotation is merged with another and both modified the same bucket, that one bucket takes precedence and not that they are merged in place by e.g. combining their changes to the fallback layer?

Merging volume annotations cannot distinguish between fallback layer voxels and annotated voxels, so for a dense fallback layer the result will always look weird. For each voxel, one of the annotations wins, but since both have dense voxels in their annotated buckets, all these voxels will win over whatever the other did there. By introducing zeroed (black) voxels, the behavior changes. So yes, this is expected. When merging two fresh volume annotations (with a lot of zeroes), you should see voxel-wise merging.

I pushed a fix for the duplicate bug. I’ll have a look at the rest.

[fm3]

quoting michael from above:

preview the version where the agglomerate mapping was activated -> The frontend crashed due to the unexpected server 400 error.

I think this is a frontend problem. This route should not be queried at all for annotation versions before the editable mapping was created.

I think the compound task bug is backend. Everything else looks like frontend to me, but I’m not 100% certain. @philippotto could you have a look please?

If possible, it would be nice to propagate the error properly to show it to the user in an understandable way. The message is already clear, but too hidden for the user. But I think this is unrelated -> maybe open an issue / pr for this

True, that would be nicer. But I think most people would click »more info« in this case and are able to find the real error. So I’d call that low priority. So far, nobody complained about this.

[MichaelBuessemeyer]

True, that would be nicer. But I think most people would click »more info« in this case and are able to find the real error. So I’d call that low priority. So far, nobody complained about this.

Ok I created an issue for this: https://github.com/scalableminds/webknossos/issues/8332

[philippotto]

@MichaelBuessemeyer thanks for your thorough testing :+1: I fixed a couple of front-end bugs (mostly the ones you found but also some others).

Now the old / initial volume annotation layer with the editable mapping is not enabled / visible by default an for each of these versions one must enable it so see it. This is kinda annoying. I don't know, whether this is related to this pr though or whether this should be handled in a separate pr

This only seems to happen when that volume layer is invisible before the restore view is opened. Therefore, I'd argue that the priority is minimal (small possibility + small impact). Also, it might be unrelated to this PR. Therefore, I'd defer this for now.

---

I talked with @fm3 and we found a bug when deleting a volume layer (or editable mapping) and reverting to another version where it exists again. After this is fixed, we should go through all your repro steps from above to make sure everything is okay.

[MichaelBuessemeyer]

I talked with @fm3 and we found a bug when deleting a volume layer (or editable mapping) and reverting to another version where it exists again. After this is fixed, we should go through all your repro steps from above to make sure everything is okay.

Oh, good thing you found that :tada:. Just ping me when I should take another look :D :eye:

[fm3]

ping :) I hope I fixed those problems in 51839f0

[philippotto]

https://github.com/scalableminds/webknossos/commit/596a37430396dada0b8d868a230d9714d17bccc3 introduces changes to avoid the frequent clicking bug I found? At least that how the code kinda reads :)

yes :)

[MichaelBuessemeyer]

The newest changes look also good to me :) Thanks for incorporating them. I also did some slight unstructured testing (tried merging annotations, compound tasks & compound projects)

[MichaelBuessemeyer]

I did some more testing (went trough WK Testing steps and did want I did not yesterday):

While testing I noticed that a single proofreading action results in 3 version entries. Is it maybe possible to reduce this to one version entry?

Sorry but I found another little frontend bug:

• [x] https://github.com/user-attachments/assets/2e4a65fa-ce8b-4784-8720-90bc82a427e6 Maybe the reason for this bug is that I selected the newest version and then closed the view :thinking:?

[philippotto]

While testing I noticed that a single proofreading action results in 3 version entries. Is it maybe possible to reduce this to one version entry?

we can probably slim this down, but let's do it in a follow up. I created https://github.com/scalableminds/webknossos/issues/8345

https://github.com/user-attachments/assets/2e4a65fa-ce8b-4784-8720-90bc82a427e6 Maybe the reason for this bug is that I selected the newest version and then closed the view 🤔?

thanks for spotting this! I fixed this now. please have a look at my latest commit. I tried to clean up how the allowUpdate property is restored. the original problem was that useWillUnmount didn't have a dependency on the initialAllowUpdate property (and that could be set to false because of the dummy annotation in the default state).