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

Store editable mappings in multiple fossildb columns+keys #6903

Merged
merged 56 commits into from
Apr 26, 2023

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Mar 6, 2023

URL of deployed dev instance (used for testing):

TODO

  • on multiple update actions, use temporary store, flush to fossil on commit
  • migration
  • problem: newest version may not be materialized, do we apply update actions in migration? or should the code be able to apply lazily?
  • test perf
  • how to apply migration in production? how much downtime is needed?
  • two-phase migration to lower downtime?
  • include relevant changes from WIP: Profile Proofreading Performance #6829
  • finalize migration (args, camel_case)
  • Reproduce philipps failed mesh requests → cannot reproduce, let’s defer.
  • Front-end should not send fallback layer requests if volume tracing bucket requests fail
  • should action with segment id 0 also become no-op? yes

Steps to test:

  • Create annotation with volume layer with segmentation fallback (that has an agglomerate file. on the dev instance I suggest l4dense_motta_et_al_dev_v2)
  • Select an agglomerate mapping and use the proofreading tool to split and merge some cells
  • refresh the page, should still be look right
  • To test the migration locally, repeat above steps on master, then switch to this branch, start the fossildb (yarn start does it), then run the migration python tools/migrate-editable-mappings/migrate-editable-mappings.py -vw localhost:7155 , then reload the same annotation and see that everything is there and still works as expected

Issues:


  • Updated migration guide if applicable
  • Removed dev-only changes like prints and application.conf edits
  • Considered common edge cases
  • Needs datastore update after deployment

@philippotto
Copy link
Member

Great stuff! Testing of proofreading went mostly well 👍

However, during merging/splitting in proofreading mode, I occassionally ran into failed network requests:
image
The visible meshes look alright, but maybe I wouldn't notice a missing chunk.

Unfortunately, I cannot really figure out what's going wrong. Since there is no status code, I'm not even sure the request arrived at the server? The failed requests point towards https://editablemappingsdistributed.webknossos.xyz/data/datasets/sample_organization/l4dense_motta_et_al_dev_v2/layers/segmentation/meshes/formatVersion/3/chunks/data. Maybe you have an idea?

Next, I'll test the migration.

@philippotto
Copy link
Member

The migration worked flawlessly 👍

@fm3
Copy link
Member Author

fm3 commented Apr 19, 2023

Hmm, I’m not sure about that. The chunk data request did not change in this PR. However, the chunk list request may have changed (since it has to list all chunks of the segments currently mapped to that particular agglomerate id) so there may be invalid byte offsets there? However, I don’t see how that would not lead in a 4xx or 5xx but this strange behavior of your screenshot 🤔 I could not reproduce this behavior.

@fm3
Copy link
Member Author

fm3 commented Apr 19, 2023

I think I found an unrelated problem, where I cannot perform a particular split 🤔 I’ll have a look at that

@philippotto
Copy link
Member

What would be the rollback strategy for this PR? I just had some trouble working on other branches, since fossilDB complains that I'm not opening the new column families (which don't exist on the other branches).

How did you solve this while branch hopping? I deleted fossildb/data, but this is obviously not possible in production 😅

@fm3
Copy link
Member Author

fm3 commented Apr 21, 2023

This was also my strategy locally. In production, we would have to continue opening the full list of column families. This already works for the dev deployment fossildb (which is shared by all dev deployments)

@fm3
Copy link
Member Author

fm3 commented Apr 24, 2023

I think I found an unrelated problem, where I cannot perform a particular split thinking I’ll have a look at that

It turned out to be caused by mag1 not being present. The agglomerate file had a node very close to the edge of the agglomerate. The voxel there was eaten by downsampling (the other segment id won at that position). That meant that looking up the segment id by the position from the agglomerate file lead to the wrong segment id, essentially making the split a no-op since the edge was not actually present in the agglomerate. I’d say that this is acceptable since it only happens in case of a mismatch between the present segmentation data and the agglomerate file. I fear there is no good way to guard against this. I added some warning logging in case an edge is not present and a split becomes a no-op.

@fm3
Copy link
Member Author

fm3 commented Apr 24, 2023

@philippotto do you think we can go ahead with this? Can you still reproduce the issue with mesh loading? Maybe you could show me in person?

@philippotto
Copy link
Member

@philippotto do you think we can go ahead with this? Can you still reproduce the issue with mesh loading? Maybe you could show me in person?

As discussed in person, I think we can go ahead with this. I also fixed the incorrect fallback fetching case.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

💯

@fm3 fm3 merged commit 29ea776 into master Apr 26, 2023
@fm3 fm3 deleted the editable-mappings-distributed branch April 26, 2023 07:23
@fm3 fm3 restored the editable-mappings-distributed branch April 26, 2023 12:32
@fm3 fm3 deleted the editable-mappings-distributed branch April 26, 2023 12:52
hotzenklotz added a commit that referenced this pull request Apr 28, 2023
…ove_wkconnect

* 'master' of github.com:scalableminds/webknossos:
  Update docker compose commands + dev install readme (#7002)
  Add segment groups (#6966)
  Add screenshot nightly test for wkorg (#7030)
  Workaround for WebGL crash for datasets with many segmentation layers (#6995)
  Fix download of public annotation, include access ctx in user cache key (#7025)
  Fix that changing a segment color could lead to a crash (#7000)
  Add more error chaining to annotation download (#7023)
  Guard against NaNs in shader (#7018)
  Store editable mappings in multiple fossildb columns+keys (#6903)
  Context action to move tree to group (#7005)
  Release 23.05.0 (#7014)
  Remove vault cache when reloading dataset (#7007)
  Fix viewing of public datasets (#7010)
  Update screenshots scalebar positioning (#7003)
  Update team members (#6999)
@fm3 fm3 mentioned this pull request May 2, 2023
7 tasks
hotzenklotz added a commit that referenced this pull request May 17, 2023
…ty-list-drawings

* 'master' of github.com:scalableminds/webknossos: (25 commits)
  Fix issues with styling in dark mode on login page (#7052)
  Fix nightly by setting missing token (#7048)
  Release 23.05.1 (#7042)
  DRY types in update_actions.ts (#7036)
  Remove some spammy logging from backend (#7039)
  Use zarr string fill values (#7017)
  Fix voxel offset for Neuroglancer Precomputed datasets (#7019)
  Log when user is activated (#7027)
  Fix exception in applying UpdateTreeGroupVisibility skeleton action (#7037)
  Fix organization storage layouting (#7034)
  Update docker compose commands + dev install readme (#7002)
  Add segment groups (#6966)
  Add screenshot nightly test for wkorg (#7030)
  Workaround for WebGL crash for datasets with many segmentation layers (#6995)
  Fix download of public annotation, include access ctx in user cache key (#7025)
  Fix that changing a segment color could lead to a crash (#7000)
  Add more error chaining to annotation download (#7023)
  Guard against NaNs in shader (#7018)
  Store editable mappings in multiple fossildb columns+keys (#6903)
  Context action to move tree to group (#7005)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Measure + Improve server-side performance for supervoxel proofreading
2 participants