Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transactional saving of volume annotations #7264

Merged
merged 24 commits into from
Aug 21, 2023
Merged

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Aug 14, 2023

Description:

  • Introduces sensible transaction points for volume annotations (--> also means less versions which will be helpful in the version restore view 🎉). This is done by waiting for small time windows (1 second) in which no volume data was mutated and no dirty volume buckets need to be downloaded. This is similar to how the PushQueue operated, anyway, but it chunked buckets into 32 batches later which destroyed any transactional semantics. Reusing the state machine of the undo mechanism would be nicer, since it is strictly user-interaction based. However, the implementation of that would be far more complicated.
  • Improved speed of bucket compression by using larger payloads for the webworkers (an isolated benchmark with reasonable inputs showed that the bucket compression is now 5 to 10 times faster).
  • Removes AsyncTaskQueue as its sequentiality and failure-mechanism aren't needed anymore. This abstraction was originally introduced to deal with failing network requests. However, that part is not handled by the AsyncTaskQueue anymore.
  • The error handling in the PushQueue was hardened and unified with the error handling in the sagas (by escalating potential exceptions to the sagas).
  • Fixes a small bug in the save-saga which could lead to an accumulating backlog of save queue items that would grow faster than they would be saved. This could happen when brushing a high volume of data and only relying on the auto-save mechanism. That mechanism did only send 1 request every 30 seconds (even when multiple requests would be necessary to completely save the state of that time).
  • The save button now also warns when the PushQueue is starving (i.e., constantly unable to bundle up a transaction, because a bucket was edited again). Previously, this was achieved by providing a max_time parameter to _.debounce. However, since we want sensible transactions, this is not an option anymore.
  • Move some modules from libs to libs/async.
  • Lower saving batch constants for volume tracings, because the backend needs more time to process the update actions (see discussion here)
  • Improve progress indicator during saving (the tooltip for the save button will also show when buckets need to be downloaded during saving, because this can take a noticable amount of time).

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • do a volume annotation (with and without fallback)
  • set the network speed to very slow
  • reload the tab during save (large volume changes are necessary to cause multiple requests)
  • ensure that the volume data looks consistent (zoom in and out to check all mags)
  • additionally/alternatively:
    • brush a large area
    • save
    • look at the version restore view. there should be one version (maybe other versions for the new segment id and for updateTracing, but not more)
    • compare the above to master.webknossos.xyz where lots of versions will be created. previewing one version and zooming to different mags will show hard-cuts in the brushed data.

TODOs:

  • test volume speed on dev instance (which will use prod mode) to ensure that it didn't regress
  • verify that new save requests don't get too large. since updateBucket actions are way larger than skeleton update actions, some tuning might be necessary here. <-- no problem, since the compression does a good job of keeping the size small

Issues:


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

@philippotto philippotto self-assigned this Aug 14, 2023
@@ -11,5 +11,5 @@ export const UNDO_HISTORY_SIZE = 20;
export const SETTINGS_RETRY_DELAY = 15 * 1000;
export const SETTINGS_MAX_RETRY_COUNT = 20; // 20 * 15s == 5m

export const maximumActionCountPerBatch = 5000;
export const maximumActionCountPerSave = 15000;
export const maximumActionCountPerBatch = 20;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: revert

@philippotto philippotto changed the title [WIP] Transactional saving of volume annotations Transactional saving of volume annotations Aug 15, 2023
@philippotto philippotto marked this pull request as ready for review August 15, 2023 16:55
@philippotto
Copy link
Member Author

@normanrz Are you open for reviewing this PR? I'll do some additional testing on a dev instance, but the code is ready for review. It might make sense to have a call about this PR and do a review with 4 eyes, since the code is mission-critical for volume saving. Let me know if/when you have time for that 🤙

Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is a piece of art! Thanks for writing all the descriptive comments. I think I understand what is roughly going on and look forward to the walkthrough.

// That queue is flushed in a debounced manner so that the time of the
// snapshot should be suitable for a transaction (since neither WK nor the
// user edited the buckets in a certain time window).
private pendingQueue: Set<DataBucket>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the queue a Set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of the buckets doesn't matter within a transaction, which is why this is a set. However, the mismatch is obviously still an eye sore. I renamed it to pendingBuckets now. The class PushQueue might also be ripe for a new name, but then the PullQueue loses its "name-sibling" 😅 Also, I'm not sure how to call it. BucketTransactionBundler? Might be okay to keep the PushQueue as is.

@philippotto philippotto enabled auto-merge (squash) August 21, 2023 08:22
@philippotto philippotto merged commit 58f5703 into master Aug 21, 2023
1 check passed
@philippotto philippotto deleted the volume-transactions branch August 21, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes volume buckets are not properly saved
2 participants