Add load and save for JSON annotations

[subdavis]

follow up to discussion in #384

Chosen implementation

For now, the simplest option and the one that maintains the best compatibility with the server is to load and save annotation json file from the same directory as the project data. This will enable you to download a ZIP of everything from VIAME web, extract it, and open it directly with the desktop.

• The desktop will continue to use the auxiliary folder for historical saves.
• CSV load/save is unimplemented because no parser or serializer exists for VIAME CSV in Node. I'd like to potentially call out to our existing python implementation, but that may or may not be a reasonable approach.
• I might pull in my KPF, KW18, and VIAME parsers from https://github.com/girder/videothing/tree/master/packages/data/loaders
  • I wrote that code a year ago though, so it's probably garbage.

Other considerations

Because validation is done server-side in pydantic, no validation on load really exists here. Save is a bit more protected due to the typescript interface. What I'd really like to have is some kind of schema versioning strategy, so that the loader knows when a annotation file is out of date and requires an upgrade.

Tl;DR this is basically identical to how girder stores and updates annotation data. I couldn't come up with a strong argument to do something different, and consistency has value.

PTAL.

[BryonLewis]

This wasn't an exhaustive test, just some initial findings on the windows side. I'll do a more indepth review later. Main thing I think would be the regex for the json file modified so it can support windows files as well as determining where we want to set the datasetType and if we need to support individual file references instead of folders.

[BryonLewis]

Is the restrictiveness of this check because of the lack of validation? Also window's doesn't have a concept of ':' within a file name. Files downloaded from the server are manually changed to swap the ':' to a '_'.

[subdavis]

Why is windows so stupid.

[subdavis]

Fixed b35bea9

[BryonLewis]

I don't know if this will ever be triggered here. I did some testing/logging and on video types it is still a directory. Your dataset is always a folder because you can only open folders in which it grabs the first video out of the folder. If we aren't supporting multiple videos in a single folder I don't know if this is needed or the code directly below. Then in the main.ts when you load the meta data the type is changed based on the filtering and mimeTypes used. So you have two different datasetTypes set.

[subdavis]

Symlink was just the first exception I could think of.

https://stackoverflow.com/questions/15630770/node-js-check-if-path-is-file-or-directory

There are also FIFOs, block devices, and all sorts of other stuff. I should change the error message.

[subdavis]

I can't find difinitive docs on whether isFile and isDirectory are mutually exclusive 100% coverage, or if there are other cases, but I'd rather be unsure and have the error.

Edit: fifo is neither.

> fs.statSync('./fifo')
Stats {
  dev: 2049,
  mode: 4516,
  nlink: 1,
  uid: 1000,
  gid: 1000,
  rdev: 0,
  blksize: 4096,
  ino: 52561314,
  size: 0,
  blocks: 0,
  atimeMs: 1604089547817.2932,
  mtimeMs: 1604089547817.2932,
  ctimeMs: 1604089547817.2932,
  birthtimeMs: 1604089547817.2932,
  atime: 2020-10-30T20:25:47.817Z,
  mtime: 2020-10-30T20:25:47.817Z,
  ctime: 2020-10-30T20:25:47.817Z,
  birthtime: 2020-10-30T20:25:47.817Z
}
> const a = fs.statSync('./fifo')
undefined
> a.isFile()
false
> a.isDirectory()
false
> 

[subdavis]

Fixed b35bea9

[BryonLewis]

I don't think I was clear in my comment. The main issue is that it thinks everything is a image-sequence because all datasets are folders now. So when I was logging this the datasetType here would be an image-sequence but then in main.ts it would be a video. My main thing was that this whole section might not be necessary because in desktop you don't support single files. I might be wrong in my interpretation of that.

[subdavis]

Oh! Duh. Sorry I didn't pick up on this.

I'll get that fixed. Thank you.

[BryonLewis]

This may not matter. This doesn't seem to trigger for symbolic links in windows. It loads properly both the converted json data (with the regex ":" swapped to "_") and the video in the folder.

[BryonLewis]

The breakdown of the IPC stuff makes sense to me.

I wouldn't worry about it now but in the future there might be some stuff we can share between linux/windows like the pipeline constants for the types and maybe some other stuff.

I'm assuming default-settings was just a handy way to handle requesting those values. There really is no reason for it to require access through the IPC other than maintaining consistency? I like the consistency so keep it that way.

This is a just a question which may not have an answer as of now:
A defined requirement (I don't know if there are checks during install) for a VIAME installation is an Nvidia GPU with 4-8GB of memory:

Should the check for compatibility be done at the electron/viewer level or during the installation of VIAME? Then to complicate more if you go to download VIAME, you can choose that you want a Windows CPU only version (which leads me to the assumption that there exist pipelines that can be run in CPU only). In closing I'm just wondering if we are doing a check for GPU compatibility when VIAME-CLI should be doing the check given it has a GPU and CPU version? I really like what you did here because it makes it easy for the user to see if they can run GPU but we may want to have a talk about the overall structure of this and how it interfaces with VIAME-CLI stuff. Do we change available pipelines based on version installed and card support that is available?

This is just for future reference/info: nvidia-smi in windows is buried in an semi-known folder location in windows of course. There is some minor structure to the directory in which it is located but it may take some indexing of higher folders to find the right subfolder that would have the exe.

[BryonLewis]

Sorry just one small addendum. In terms of the VIAME install location there needs to a browse button and dialog to select that. You and I may be used to typing in filepaths, but windows will most definitely require a file browser tool to point to the proper location. No Windows user is going to type "C:\Users\username\Documents\NOAA\VIAME and stuff like that to indicate the location.

[subdavis]

I'm assuming default-settings was just a handy way to handle requesting those values. There really is no reason for it to require access through the IPC other than maintaining consistency? I like the consistency so keep it that way.

Default settings will be platform-specific, so they need to go through the platform switching, which I figured would be easiest to implement in a single function in the main thread, so I ran it through IPC.

Sorry just one small addendum. In terms of the VIAME install location there needs to a browse button and dialog to select that. You and I may be used to typing in filepaths, but windows will most definitely require a file browser tool to point to the proper location. No Windows user is going to type "C:\Users\username\Documents\NOAA\VIAME and stuff like that to indicate the location.

Sure, that seems reasonable.

This is just for future reference/info: nvidia-smi in windows is buried in an semi-known folder location in windows of course. There is some minor structure to the directory in which it is located but it may take some indexing of higher folders to find the right subfolder that would have the exe.

I vote we just look for the 2-3 most common fixed paths then show a warning with the shrug emoji.

Do we change available pipelines based on version installed and card support that is available?

Punting for now.

Should the check for compatibility be done at the electron/viewer level or during the installation of VIAME?

We don't control install time, and system configuration can change. I personally think it's reasonable to do a runtime check in a context we have complete control over.

That being said, I was really just stretching my legs with the spawn() interface. I don't plan on making these checks more complicated or robust than they already are.

[subdavis]

I split out the complicated settings stuff, so this is just the addition of track load and track save. #424 will follow.

I'm ready for final review and merge.

[BryonLewis]

I like the common folder, I know we may need to modify it slightly once we do windows, but I think it will be helpful.

I did some basic testing in the web version to make sure stuff really worked. About what I could test in 20 minutes (video loading, saving, editing and others) before looking at the actual electron stuff. If you feel like it is necessary I can do a more in-depth test of the web stuff.

I had one comment about error handling for saving data and permissions. It works in the fact that an error is thrown and UI responds with 'Unable to Save Data'. If fs was able to throw an error structured in the response error style it could pick up better text as to why the error happened. Like I said in the comment it can be marked as resolve.

[BryonLewis]

Interesting thing in some testing that we may want to add at some point is try/catch for permissions to write to a folder. You would need it around the aux folder creation, the file movement and possibly the writing file. If you take the current error you could mimic a response.status = 403 on it and it would return the proper error message to the user utilizing the exist stuff. I was just testing the saving and loading and permissions. Not something that is needed right now, can mark as RESOLVED.

[BryonLewis]

All went well with my additional check of the web version. Saving/loading, running pipelines, more in depth editing. I couldn't find anything that seemed wrong.

Only web-common change was the type change in the apispec.ts. We may want to export that DatasetType for the web stuff at some point to reduce the duplication of type: 'image-sequence' | 'video' done in some other areas as well as the root imageSequenceType and VideoType found in viame-web-common.constants.js. Just we seem to have a couple central locations were specify media types. That can be done in another PR though.

[subdavis]

Many of these changes are ongoing in #424, including sharing DatasetType.

I'm actively trying not to add too much polish right now just because polish has sort of a grout effect: it becomes harder to shuffle stuff around when we invariably learn that some approach won't transfer to windows well.

I'm trying to write less code for the sake of agility. there will at some point be a mega-review where we decide that the current app makes sense architecturally.

I'm glad we have your comments in this issue to revisit when that happens.