command: Show dvc list output in pretty JSON format #6647

eggqq007 · 2021-09-19T09:25:43Z

update ls init.py to handle --show-pretty-json
introduce pygments into requirements/default.txt

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Closes iterative/dvc.org#2838
Closes #6646

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

skshetry · 2021-09-20T05:30:21Z

dvc/command/ls/__init__.py

@@ -72,6 +84,11 @@ def add_parser(subparsers, parent_parser):
    list_parser.add_argument(
        "--show-json", action="store_true", help="Show output in JSON format."
    )
+    list_parser.add_argument(
+        "--show-pretty-json",


We could just prettify the JSON by default by checking if we are in isatty mode and then prettify that output.

Thanks for the suggestion! The patch was updated

skshetry · 2021-09-20T05:38:14Z

Btw, why not just do dvc list --show-json . | python -m rich.json - or dvc list --show-json | jq?

skshetry · 2021-09-20T05:53:12Z

requirements/default.txt

@@ -44,3 +44,4 @@ typing_extensions>=3.10.0.2
 fsspec[http]>=2021.8.1
 aiohttp-retry==2.4.5
 diskcache>=5.2.1
+pygments>=2.10.0


We depend on rich, so you could just do something like following:

from rich.json import JSON obj = JSON.from_data(data) ui.write(obj, styled=True)

skshetry · 2021-09-20T07:19:34Z

dvc/command/ls/__init__.py

+                if sys.stdout.isatty():
+                    from rich.json import JSON
+
+                    ui.write(JSON.from_data(entries), styled=True)
+                else:
+                    ui.write(json.dumps(entries))


Let's move this to dvc/ui/__init__.py as ui.write_json, so that we can reuse it in other parts of dvc.

Also, support for indent= and prettify=True|False that keeps the old behaviour would be nice to have. 🙂

Also, we need to bump rich requirement, as from_data is only available since 10.9.0.

Sure, that makes sense. patch updated.

skshetry · 2021-09-21T15:22:03Z

The PR looks good. @iterative/dvc, if you have any opinions if dvc should format json. Otherwise, we'll be formatting in other places as well.

efiop · 2021-09-21T17:55:33Z

dvc/command/ls/__init__.py

-                import json
-
-                ui.write(json.dumps(entries))
+                if sys.stdout.isatty():


We could hide this check under ui.write_json (if rich doesn't do that already for us) CC @skshetry

Though that would get a little confusing with prettify, as it is not clear if it should enforce it or be more like "prettify, but only if it is a tty". Still, it feels like tty check belongs to ui.

Btw, is there any significant performace hit for prettifying? If not, maybe we should indeed consider just showing pretty json by default?

Thanks for the comments!. It is hard to tell the performance gap between json.dumps and prettifying. I can do some evaluation on it later. Besides that, I tend to believe the prettifying is not the bottleneck since dvc list should fetch repo info from remote first which may spend more time.

I did two simple tests on it.

A python array has 100000 element, each element is a dict which contains 3 key-value. json.dump takes about 0.05s, rich.json.from_date takes about 2.6s

dvc list takes about 1.8s to fetch https://github.com/iterative/example-get-started while prettifying takes 0.03s

I don't think performance should be a concern. We could choose a certain threshold like 1000 or so entries where we fallback to json.dumps(indent=4) instead of prettifying it. No one should expect us to prettify the 100000 many entries.

regretfully dvc list(and others) are not yet fully optimized themselves and having 100K entries in the resulting json is not unusual for our use case, so looks like prettifying is kinda expensive to do always. Plus we have other projects depending on this json output, so that migth be slowing them down too. But isatty check kinda solves that, because if someone will be using the json output - they will redirect it somewhere anyway, so we should be safe and efficient, while being pretty. That should make the threshold approach unnecessary.

Thanks for the comments! I move the isatty into ui init.py and the patch updated. :)

1. update ls __init__.py to output pretty JSON format 2. introduce write_json function into ui __init__.py

skshetry

We discussed this today in the team, and everyone is okay with this getting merged. For consistency, we'd like this to be used in all of the other commands that has --json flags (though in a separate PR).

skshetry · 2021-09-29T06:49:39Z

@eggqq007, would you like to create a PR (or, issue) for #6647 (review)? Thanks for this PR.

eggqq007 · 2021-09-30T08:21:08Z

@eggqq007, would you like to create a PR (or, issue) for #6647 (review)? Thanks for this PR.

Sure, new issue in #6712

eggqq007 requested a review from a team as a code owner September 19, 2021 09:25

eggqq007 requested a review from efiop September 19, 2021 09:25

eggqq007 force-pushed the dev-add-show-pretty-json branch from e4a8eff to fff3317 Compare September 19, 2021 10:43

eggqq007 changed the title ~~command: Show dvc list output in pretty JSON format (#6646)~~ command: Show dvc list output in pretty JSON format Sep 19, 2021

skshetry reviewed Sep 20, 2021

View reviewed changes

eggqq007 force-pushed the dev-add-show-pretty-json branch from fff3317 to a268c79 Compare September 20, 2021 06:37

skshetry reviewed Sep 20, 2021

View reviewed changes

eggqq007 force-pushed the dev-add-show-pretty-json branch from a268c79 to 486c1b3 Compare September 20, 2021 13:39

skshetry approved these changes Sep 21, 2021

View reviewed changes

efiop reviewed Sep 21, 2021

View reviewed changes

command: Show dvc list output with pretty JSON format if output to tty

40f04df

1. update ls __init__.py to output pretty JSON format 2. introduce write_json function into ui __init__.py

eggqq007 force-pushed the dev-add-show-pretty-json branch from 486c1b3 to 40f04df Compare September 23, 2021 00:18

skshetry added the T: discussion label Sep 27, 2021

skshetry approved these changes Sep 28, 2021

View reviewed changes

isidentical approved these changes Sep 28, 2021

View reviewed changes

skshetry mentioned this pull request Sep 28, 2021

guide: --json/csv examples in Listing/Comparing Experiments iterative/dvc.org#2860

Closed

skshetry removed the T: discussion label Sep 29, 2021

skshetry merged commit fed9f08 into iterative:master Sep 29, 2021

efiop added the enhancement Enhances DVC label Oct 11, 2021

iesahin mentioned this pull request Oct 27, 2021

Check docs coverage for dvc 2.8 iterative/dvc.org#2973

Closed

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

command: Show dvc list output in pretty JSON format #6647

command: Show dvc list output in pretty JSON format #6647

eggqq007 commented Sep 19, 2021 •

edited by pmrowla

Loading

skshetry Sep 20, 2021 •

edited

Loading

eggqq007 Sep 20, 2021

skshetry commented Sep 20, 2021 •

edited

Loading

skshetry Sep 20, 2021

skshetry Sep 20, 2021

skshetry Sep 20, 2021 •

edited

Loading

eggqq007 Sep 20, 2021

skshetry commented Sep 21, 2021

efiop Sep 21, 2021

efiop Sep 21, 2021 •

edited

Loading

efiop Sep 21, 2021

eggqq007 Sep 22, 2021

eggqq007 Sep 22, 2021

skshetry Sep 22, 2021 •

edited

Loading

efiop Sep 22, 2021

eggqq007 Sep 23, 2021

skshetry left a comment •

edited

Loading

skshetry commented Sep 29, 2021 •

edited

Loading

eggqq007 commented Sep 30, 2021

command: Show dvc list output in pretty JSON format #6647

command: Show dvc list output in pretty JSON format #6647

Conversation

eggqq007 commented Sep 19, 2021 • edited by pmrowla Loading

skshetry Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry commented Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry commented Sep 21, 2021

Choose a reason for hiding this comment

efiop Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry left a comment • edited Loading

Choose a reason for hiding this comment

skshetry commented Sep 29, 2021 • edited Loading

eggqq007 commented Sep 30, 2021

eggqq007 commented Sep 19, 2021 •

edited by pmrowla

Loading

skshetry Sep 20, 2021 •

edited

Loading

skshetry commented Sep 20, 2021 •

edited

Loading

skshetry Sep 20, 2021 •

edited

Loading

efiop Sep 21, 2021 •

edited

Loading

skshetry Sep 22, 2021 •

edited

Loading

skshetry left a comment •

edited

Loading

skshetry commented Sep 29, 2021 •

edited

Loading