Skip to content

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

Merged
skshetry merged 1 commit into
treeverse:masterfrom
eggqq007:dev-add-show-pretty-json
Sep 29, 2021
Merged

command: Show dvc list output in pretty JSON format#6647
skshetry merged 1 commit into
treeverse:masterfrom
eggqq007:dev-add-show-pretty-json

Conversation

@eggqq007

@eggqq007 eggqq007 commented Sep 19, 2021

Copy link
Copy Markdown
Contributor
  1. update ls init.py to handle --show-pretty-json
  2. introduce pygments into requirements/default.txt

Closes treeverse/dvc.org#2838
Closes #6646

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

@eggqq007
eggqq007 requested a review from a team as a code owner September 19, 2021 09:25
@eggqq007
eggqq007 requested a review from efiop September 19, 2021 09:25
@eggqq007
eggqq007 force-pushed the dev-add-show-pretty-json branch from e4a8eff to fff3317 Compare September 19, 2021 10:43
@eggqq007 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
Comment thread dvc/command/ls/__init__.py Outdated

@skshetry skshetry Sep 20, 2021

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 could just prettify the JSON by default by checking if we are in isatty mode and then prettify that output.

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.

Thanks for the suggestion! The patch was updated

@skshetry

skshetry commented Sep 20, 2021

Copy link
Copy Markdown
Collaborator

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

Comment thread requirements/default.txt Outdated

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 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)

@eggqq007
eggqq007 force-pushed the dev-add-show-pretty-json branch from fff3317 to a268c79 Compare September 20, 2021 06:37
Comment thread dvc/command/ls/__init__.py Outdated
Comment on lines 41 to 46

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.

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. 🙂

@skshetry skshetry Sep 20, 2021

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.

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

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.

Sure, that makes sense. patch updated.

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

Copy link
Copy Markdown
Collaborator

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.

Comment thread dvc/command/ls/__init__.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@efiop efiop Sep 21, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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.

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.

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.

I did two simple tests on it.

  1. 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

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

@skshetry skshetry Sep 22, 2021

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.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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.

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 skshetry left a comment

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 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

skshetry commented Sep 29, 2021

Copy link
Copy Markdown
Collaborator

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

@eggqq007

Copy link
Copy Markdown
Contributor Author

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

Sure, new issue in #6712

@efiop efiop added the enhancement Enhances DVC label Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhances DVC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

introduce --show-pretty-json into dvc list options introduce --show-pretty-json into dvc list options

4 participants