Skip to content

Fix #1052 import json and meta json#1053

Merged
subdavis merged 4 commits into
mainfrom
feature/desktop-config
Nov 17, 2021
Merged

Fix #1052 import json and meta json#1053
subdavis merged 4 commits into
mainfrom
feature/desktop-config

Conversation

@subdavis
Copy link
Copy Markdown
Contributor

@subdavis subdavis commented Nov 16, 2021

Changes

  • Fixes bug where every json file was being parsed as a NIST file and failing.
  • Adds feature to import meta json on desktop for parity with web.

Notes

  • Removes 2 large blocks of code that were inline-copies of other existing functions.
  • Replace annotationImport with dataFileImport agnostic to the kind of data being ingested to match web logic
  • Add tests.

Testing

Tested with json tracks, meta files, and CSV. I don't have any NIST data, but if you tell me where to find some, I can check that too.

fixes #1052

@subdavis subdavis changed the title WIP import json Fix #1052 import json and meta json Nov 17, 2021
@subdavis subdavis marked this pull request as ready for review November 17, 2021 15:31
@subdavis subdavis requested a review from BryonLewis November 17, 2021 15:31
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

This cleans up quite a bit of stuff.
One change noted is there was previously a guard to only allow NIST import on video files. This was done because the core NIST format relies specifically on named video files and frame numbers. I don't think this needs to be added back in.

The important issue is the possible clobbering of the project meta.json file by doing a merge with a full export meta.json from the server. If you export the configuration file and use it that works perfectly. If you do a full zip export and use the meta.json that is where there are possible failures.

const data = await nistSerializers.loadNistFile(path);
tracks = data.tracks;
meta.fps = data.fps;
} else if (DatasetMetaMutableKeys.some((key) => key in jsonObject)) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We need to restrict to only the DatasetMetaMutableKeys. If you do a full export from the server you get a meta.json file. If you import that file it passes this check but then merges with the existing meta structure causing the file to have a bunch of extra fields which may be incorrect if there is any overlap (like fps and type fields). This could also occur if a user tries to import their meta.json desktop file from another or similar dataset. It will do the merge and completely break the data then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

// eslint-disable-next-line @typescript-eslint/no-unused-vars
const console = new Console(process.stdout, process.stderr);

const emptyCsvString = '# comment line\n# metadata,fps: 32,"whateever"\n#comment line';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

very minor can ignore because it is in a test. whateever -> whatever

@subdavis subdavis requested a review from BryonLewis November 17, 2021 17:45
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

👍

@subdavis subdavis merged commit 0e4e0b0 into main Nov 17, 2021
@subdavis subdavis deleted the feature/desktop-config branch November 17, 2021 18:04
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.

[BUG] Annotation and config import in desktop not working.

2 participants