Mongo 1: Server side implementation by subdavis · Pull Request #1066 · Kitware/dive

subdavis · 2021-11-22T14:28:09Z

Reviewer notes

This is the largest change and will require the most discussion
It leaves jobs (pipelines/training/summary) in a broken state fixed by Mongo 3: Utilize new endpoints in celery #1068, but I spearated them because the changes are pretty much completely independent, and I thought it would be easier to review by breaking it apart.

Design

This is a soft delete implementation that tolerates data duplication in favor of defending against data loss.

By modeling the annotations with a rev_created and rev_deleted, you can check out any point in history. This is the simplest implementation I could come up with, and should be looked at critically. I'm open to changing it.
There is also a revision log collection. This collection is non-critical (doesn't need to be joined in order to reconstruct history). It's just a convenience for attaching metadata to revisions, such as author, description, and timestamp.

Logic - Creation

revision is an increasing integer key (per dataset)
When a track is saved, the full track is saved as a single mongo record. rev_created is set as the current revision
If a previous version of the track existed, its rev_deleted field is set, marking that previous version as lazy-deleted.

Logic - Checkout

{
        DATASET: dsFolder['_id'],
        REVISION_CREATED: {'$lte': head},
        '$or': [{REVISION_DELETED: {'$gt': head}}, {REVISION_DELETED: {'$exists': False}}],
    }

"Get annotations from this dataset where revision_created is less than or equal to target and either deleted is greater than target or deleted is not set."

Deconflicting

There's another concern we could address here: deconflicting multi-user saves. Suppose User A loads, User B Loads, User B saves, and User A saves. Currently, each save is an overwrite and User A might break some of B's work.

Now that we have revisions, each save could require that the user provide the ID they loaded and if the ID has changed since they loaded, the server could attempt to merge and detect a collision. This could be considered in-scope, but I think maintaining the existing behavior for now is best.

There are even advanced options where we could utilize the notification stream to deliver notifications to all users who have a given dataset open. This is well out of scope for this stage of changes, but interesting to think about.

BryonLewis

I'm not done reviewing but I wanted to log some comments (in the code) before I complete it.
This is an list for myself for stuff to look back at:

Need to look a bit more into the patching and seeing if I can break that.
/process_items - I need to look a bit more into that for corner cases regarding the track.json and others.
migration - need to look closer at the script for migration

BryonLewis · 2021-12-02T20:31:37Z

+
+    # Write the revision to the log
+    additions = len(insert_operations)
+    deletions = len(update_operations) - additions


could this number be negative? update_operations during an overwrite=True would never get a length if the only items were inserted? So the log_entry would have a -additions value? This I think is true during any processing of a imported dataset. I've confirmed this by looking at the revision log after uploading new data.

yep, I messed this up

BryonLewis · 2021-12-02T21:00:22Z

+        .pagingParams("revision", defaultLimit=20)
+        .modelParam("folderId", **DatasetModelParam, level=AccessType.READ)
+    )
+    def get_revisions(self, limit: int, offset: int, sort, folder):


just wondering if the default sort order should be descending instead of ascending? In actual usage may want the default to be the date/ltime/description of the last 20 instead of the first 20. This is further influenced by the default behavior of get_revisions which I think defaults to descending. So the endpoint defaults to ascending but the function defaults to ascending.

I see that this was added to the GET /dive_dataset which is probably a good idea. I meant for it to be added to the revisions here GET /dive_annoation/revision

BryonLewis

I don't know how much migrations.py was supposed to be an example vs an actual path to migration. I left a few comments on it.

subdavis · 2021-12-08T20:15:55Z

Responded to comments.

* Poetry fix * Add check for app root

BryonLewis

Additionally you may want to add in the poetry fix for the web server so this can be docker-composed and run for testing.

BryonLewis · 2021-12-09T14:22:11Z

+        .pagingParams("revision", defaultLimit=20)
+        .modelParam("folderId", **DatasetModelParam, level=AccessType.READ)
+    )
+    def get_revisions(self, limit: int, offset: int, sort, folder):


I see that this was added to the GET /dive_dataset which is probably a good idea. I meant for it to be added to the revisions here GET /dive_annoation/revision

* init * Allowing multicam to write tracks * inut name change * lint fixes * Updates to multicam * fixing import loading * removing multicamImageFiles change * Fixing various issues * mend * mend * switching to every * Example without fetching metadata (#1088) Co-authored-by: Brandon Davis <brandon.davis@kitware.com>

BryonLewis

Pulled and checked my comments, did some more basic testing and it looks good.

* Poetry fix (#1087) * Poetry fix * Add check for app root * Desktop/sealion multicam (#1024) * init * Allowing multicam to write tracks * inut name change * lint fixes * Updates to multicam * fixing import loading * removing multicamImageFiles change * Fixing various issues * mend * mend * switching to every * Example without fetching metadata (#1088) Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * Server-side implementation * Include description and timestamp * Respond to comments * Switch to checking mongo results * select sub-element * Unmangle indices Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>

* Mongo 1: Server side implementation (#1066) * Poetry fix (#1087) * Poetry fix * Add check for app root * Desktop/sealion multicam (#1024) * init * Allowing multicam to write tracks * inut name change * lint fixes * Updates to multicam * fixing import loading * removing multicamImageFiles change * Fixing various issues * mend * mend * switching to every * Example without fetching metadata (#1088) Co-authored-by: Brandon Davis <brandon.davis@kitware.com> * Server-side implementation * Include description and timestamp * Respond to comments * Switch to checking mongo results * select sub-element * Unmangle indices Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com> * Mongo 2: Rollback API (#1067) * Implement rollback * Switch to POST /dive_annotation/rollback * Mongo 3: Utilize new endpoints in celery (#1068) * Utilize new endpoints in celery * Respond to comments * Linting, formatting, and unit tests * respond to comments * Import shutil * Client changes to support revisions (#1070) * Remove broken summary and report generation (#1071) * Add simple sharing test and new indices * Migraction script updates * Add loading state to clone button * label fetch Co-authored-by: BryonLewis <61746913+BryonLewis@users.noreply.github.com>

subdavis added this to the Annotations in MongoDB milestone Nov 22, 2021

This was referenced Nov 22, 2021

[FEATURE] [Housekeeping] Move track data into Mongo #306

Closed

Mongo 2: Rollback API #1067

Merged

subdavis requested review from BryonLewis and marySalvi November 22, 2021 20:28

waxlamp modified the milestones: Annotations in MongoDB, Sprint November 30 - December 14 Nov 30, 2021

BryonLewis reviewed Dec 2, 2021

View reviewed changes

BryonLewis reviewed Dec 3, 2021

View reviewed changes

Comment thread server/scripts/migrations.py

Comment thread server/scripts/migrations.py

subdavis force-pushed the mongo/server-side-implementation branch from 337d8c6 to 1578509 Compare December 8, 2021 20:13

subdavis requested a review from BryonLewis December 8, 2021 20:15

Poetry fix (#1087)

fdb8922

* Poetry fix * Add check for app root

BryonLewis requested changes Dec 9, 2021

View reviewed changes

waxlamp modified the milestones: Sprint November 30 - December 14, Sprint December 14 - December 28 Dec 14, 2021

subdavis added 3 commits December 19, 2021 15:46

Server-side implementation

ba1392b

Include description and timestamp

43e49d2

Respond to comments

477f794

subdavis force-pushed the mongo/server-side-implementation branch from 1578509 to 477f794 Compare December 19, 2021 20:46

subdavis added 3 commits December 19, 2021 16:04

Switch to checking mongo results

5922f72

select sub-element

9880539

Unmangle indices

3eb89d7

BryonLewis self-requested a review December 20, 2021 13:22

BryonLewis approved these changes Dec 20, 2021

View reviewed changes

subdavis merged commit d677718 into develop-mongo-tracks Dec 20, 2021

subdavis deleted the mongo/server-side-implementation branch December 20, 2021 14:07

subdavis mentioned this pull request Jan 18, 2022

Migrate track data to MongoDB #1104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mongo 1: Server side implementation#1066

Mongo 1: Server side implementation#1066
subdavis merged 8 commits into
develop-mongo-tracksfrom
mongo/server-side-implementation

subdavis commented Nov 22, 2021 •

edited

Loading

Uh oh!

BryonLewis left a comment

Uh oh!

BryonLewis Dec 2, 2021

Uh oh!

subdavis Dec 3, 2021

Uh oh!

BryonLewis Dec 2, 2021 •

edited

Loading

Uh oh!

BryonLewis Dec 9, 2021

Uh oh!

Uh oh!

BryonLewis left a comment

Uh oh!

Uh oh!

Uh oh!

subdavis commented Dec 8, 2021

Uh oh!

BryonLewis left a comment

Uh oh!

BryonLewis Dec 9, 2021

Uh oh!

Uh oh!

BryonLewis left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

subdavis commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer notes

Design

Logic - Creation

Logic - Checkout

Deconflicting

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

BryonLewis Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

subdavis Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

BryonLewis Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryonLewis Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

subdavis commented Dec 8, 2021

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

BryonLewis Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BryonLewis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

subdavis commented Nov 22, 2021 •

edited

Loading

BryonLewis Dec 2, 2021 •

edited

Loading