Command line tools by subdavis · Pull Request #825 · Kitware/dive

subdavis · 2021-06-30T13:04:53Z

Adds coco and viame to dive conversion
Adds dataset generation
Adds DIVE json file verification against pydantic schema.

BryonLewis · 2021-06-30T15:06:43Z

I don't know if you want this updated version of generateLargeDatasey.py, No guarantees on functionality but it allows for creation of videos as well, and I now modified the image/video so they have some noise to display. I used this to generate data for the long loading PR testing.

https://gist.github.com/BryonLewis/ac74e93f954eba591ed76cd84c954be4

Updates:

--video option to indicate you want a video
--width/--height options to specify size of image or size of video, all bboxes are still full frame though
--fps indicator for video framerate.

subdavis · 2021-06-30T15:50:29Z

This is probably going to go stale for a while, I'll deal with updating it later.

f4str · 2021-07-09T15:25:05Z

server/scripts/cli.py

+    click.echo(f'wrote attrib {output_attrs.name}')
+
+
+@convert.command(name="viame2dive")


It might also be nice to also add a dive2viame command since the export csv serializer already exists.

f4str · 2021-07-09T15:27:06Z

server/scripts/generateLargeDataset.py

 import cv2
 import numpy as np
-from PIL import Image
 from tqdm import tqdm


numpy and tqdm should probably also be added to the dev requirements.

f4str · 2021-07-09T15:39:05Z

server/scripts/cli.py

+
+from dive_server.serializers import kwcoco, viame
+from dive_utils import models
+from scripts import generateLargeDataset


generateLargeDataset imports numpy, opencv, and tqdm and since cli.py imports generateLargeDataset, it also require those imports. This isn't an issue if the dev requirements are installed, but if the user wants to use the CLI, they would need the dev requirements.

f4str · 2021-07-09T15:40:45Z

server/setup.py

    version="1.4.1",
    zip_safe=False,
    entry_points={
+        "console_scripts": ["dive=scripts.cli:cli"],


We should probably update the README with a really brief section that the CLI exists and how to use it.

server/dive_utils/models.py

subdavis · 2021-07-21T16:43:47Z

Updated changes:

drop notebooks completely.
move serializers to dive_utils to prevent the command line from depending on the dive_server package (and because I think this is the correct place to put it anyway)
Add dev and normal command lines, so that we can build a standalone that doesn't require cv2 and numpy

f4str · 2021-07-21T17:52:39Z

server/scripts/generateLargeDataset.py

+):
+    os.mkdir(directory)
+
+    with click.progressbar(range(int(frames))) as bar:


If frames is already an integer, is there a reason to cast it again?

f4str · 2021-07-21T17:56:28Z

server/scripts/generateLargeDataset.py

+            tracks[track] = track_obj
+    json_file = f"{directory}/result_test.json"
+    with open(json_file, "w") as outfile:
+        json.dump(tracks, outfile)


Might be nitpicking here, but since the other cli converters use indent=4, this should also probably have that to be consistent. Also makes viewing the output json way easier for anyone who uses it.

should probably remove from both, it adds a lot of bytes to the file

Agreed, it's probably better to have all json outputs be 1 line and compressed so people's text editors don't crash when opening massive files.

f4str · 2021-07-21T17:58:18Z

server/setup.py

+    "numpy",
+    "opencv-python",
    "pytest",
+    "pytest-cov",


Is there a reason to add pytest-cov to the dev requirements? It doesn't seem to be used anywhere and if it is needed, tox can just automatically install it.

f4str · 2021-07-21T18:01:17Z

server/setup.py

    include_package_data=True,
    zip_safe=False,
    entry_points={
+        "console_scripts": ["dive=scripts.cli_dev:cli"],


This is using the dev cli. We should probably have two entry points for console scripts (one for standard and one for dev) like this:

"console_scripts": ["dive=scripts.cli:cli", "dive-dev=scripts.cli_dev:cli"],

There's no reason to expose both, IMO. The dev cli includes everything. The limited CLI is just so pyinstaller doesn't bundle numpy

Got it. In that case, we should probably specify somewhere that the cli requires dev requirements.

f4str · 2021-07-21T18:23:21Z

server/tox.ini


+[testenv:buildcli]
+extra =
+    dev


If we're packaging the non-dev cli, there's no reason to include the dev requirements. This also brings the question if you should be able to build just one or both the dev and non-dev. tox can probably have a switch flag to choose. Alternatively, we can have two different tox options (buildcli and buildcli_dev).

Should only be able to build the non-dev

Got it. Then you can just remove the

extra = dev

This won't make a difference in the pyinstaller output, but by doing this tox won't need to install numpy and opencv just to setup the environment (which isn't needed for creating the non-dev cli). No effect on functionality, but will save some time.

f4str · 2021-07-21T18:36:55Z

server/tox.ini

+deps =
+    pyinstaller
+commands =
+    pyinstaller --onefile scripts/cli.py


This doesn't seem to work correctly since the generated output file doesn't have any options other than --help and --version. I'm not sure if this still WIP or actually has a bug. I'm suspecting it might be something to do with the cli.py file not importing stuff correctly for pyinstaller to use in click.

Also be sure to add {posargs} at the end so additional optional flags can be passed.

Had to blow away my tox env to reproduce. rm -rf .tox && tox -e buildcli && ./dist/cli . Something I had before was making this work.

f4str · 2021-07-21T18:41:52Z

server/scripts/cli.py

+    pass
+
+
+import scripts.commands  # noqa: E402 F401


This is kind of hacky to import all the click commands and rely on the import registering all the click commands. There are much better ways to separate out the commands and cli files.

The top answer in this stackoverflow post is pretty good:
https://stackoverflow.com/questions/34643620/how-can-i-split-my-click-commands-each-with-a-set-of-sub-commands-into-multipl

But I'd recommend doing something similar to what girder is doing:
https://github.com/girder/girder/tree/master/girder/cli

This also might be the reason why the pyinstaller build doesn't work.

f4str · 2021-07-22T16:41:52Z

This is great. I was emphasizing the {posargs} in tox so you can specify additional parameters. For example

tox -e buildcli -- --distpath path/to/output --clean --log-level DEBUG

Everything else seems to work great. I ran some tests and validation, conversion, and data generation all work perfectly.

Just a few minor points

As mentioned before, we should decide whether to remove the indent=4 from the viame2dive and coco2dive so the files don't get bloated
I noticed running
```
dive --version
```
doesn't seem to work and crashes. Looks like click can't figure out the package distribution correctly for the server/scripts directory or something. Probably doesn't matter too much, but the work around is really simple. In server/scripts/__init__.py make the replacement
```
@click.version_option(1.0)
```
to manually pass in a version number.
sever/testutils is pretty much empty except for 2 json files which are only used in unit tests. One option is to just move them to server/tests, but this isn't that big of an issue.

f4str · 2021-07-22T17:19:46Z

Update on the --version flag issue. Much simpler fix. According to https://click.palletsprojects.com/en/8.0.x/api/#click.version_option just specify the python package name

@click.group()
@click.version_option(package_name='dive_server')
def cli():
    pass

If you don't want the version number being displayed at all, add the message parameter. Source: https://github.com/girder/girder/blob/master/girder/cli/__init__.py

@click.group()
@click.version_option(package_name='dive_server', message='%(package)s')
def cli():
    pass

subdavis · 2021-07-27T16:55:38Z

@f4str I addressed those comments.

indent=4 was kept for the attributes files and removed for the track files. The attributes files will always be small.
testutils should just be json files now

f4str

Looks great! Tested it out (including --version) and everything runs without issue).

stale

subdavis · 2021-07-29T14:17:32Z

@BryonLewis would you like to review again?

subdavis force-pushed the command-line-tools branch from 2d79301 to b797d3b Compare July 8, 2021 19:04

subdavis marked this pull request as ready for review July 8, 2021 20:57

subdavis requested review from BryonLewis and f4str and removed request for f4str July 8, 2021 20:57

f4str suggested changes Jul 9, 2021

View reviewed changes

subdavis added this to the Sprint July 13 - July 27 2021 milestone Jul 12, 2021

BryonLewis previously requested changes Jul 12, 2021

View reviewed changes

server/dive_utils/models.py Outdated Show resolved Hide resolved

server/dive_utils/models.py Show resolved Hide resolved

subdavis self-assigned this Jul 15, 2021

subdavis and others added 8 commits July 20, 2021 10:53

WIP

4d0e269

Update with ykey

80bc017

Drop logs

e11bb31

Don't ditch notebooks just yet

0c2a872

Add converters

7e2f749

Lint

5b58e77

Respond to comments

9cbdf25

Finish cli stuff

6b649c8

subdavis force-pushed the command-line-tools branch from cf702e2 to 6b649c8 Compare July 21, 2021 16:37

Undo move function

17c64b0

subdavis requested review from BryonLewis and f4str July 21, 2021 16:43

f4str suggested changes Jul 21, 2021

View reviewed changes

subdavis added 2 commits July 21, 2021 15:01

Fix build

677e8d6

pyinstaller args

dfac6b3

subdavis requested a review from f4str July 22, 2021 15:46

subdavis modified the milestones: Sprint July 13 - July 27 2021, Sprint July 27 - August 10 Jul 27, 2021

Version comment

6af1635

f4str approved these changes Jul 27, 2021

View reviewed changes

BryonLewis approved these changes Jul 30, 2021

View reviewed changes

subdavis merged commit 5375ec4 into main Jul 30, 2021

subdavis deleted the command-line-tools branch July 30, 2021 13:50

		click.echo(f'wrote attrib {output_attrs.name}')


		@convert.command(name="viame2dive")

Conversation

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

Uh oh!

Uh oh!

BryonLewis commented Jun 30, 2021

Uh oh!

subdavis commented Jun 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f4str Jul 21, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

f4str commented Jul 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

subdavis commented Jul 27, 2021

Uh oh!

f4str left a comment

Choose a reason for hiding this comment

Uh oh!

subdavis commented Jul 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

subdavis commented Jun 30, 2021 •

edited

Loading

subdavis commented Jul 21, 2021 •

edited

Loading

f4str Jul 21, 2021 •

edited

Loading

f4str commented Jul 22, 2021 •

edited

Loading