Skip to content

Add stats --json output#377

Merged
pablogsal merged 4 commits into
bloomberg:mainfrom
oddsun:add-stats-json-cli-arg
May 22, 2023
Merged

Add stats --json output#377
pablogsal merged 4 commits into
bloomberg:mainfrom
oddsun:add-stats-json-cli-arg

Conversation

@oddsun
Copy link
Copy Markdown
Contributor

@oddsun oddsun commented Apr 25, 2023

Closes: #187

Describe your changes
Added json flag and implementation to allow export stats to json file using memray stats --json ....

Testing performed
Added 2 unit tests

  • tests/unit/test_stats_reporter.py::test_stats_output: test printing stats to terminal
  • tests/unit/test_stats_reporter.py::test_stats_output_json: test output json stats

Additional context
pycon2023

@oddsun oddsun changed the title Add stats json cli arg Add stats --json output Apr 25, 2023
@oddsun oddsun marked this pull request as ready for review April 25, 2023 21:07
@oddsun oddsun force-pushed the add-stats-json-cli-arg branch 3 times, most recently from 6af27c1 to 016d619 Compare April 27, 2023 02:10
@oddsun
Copy link
Copy Markdown
Contributor Author

oddsun commented Apr 27, 2023

new forced-push:

  • rebased to head of main
  • fixed news issue
  • fixed mypy errors

@oddsun oddsun force-pushed the add-stats-json-cli-arg branch from 016d619 to 0bc1567 Compare April 27, 2023 02:43
@oddsun
Copy link
Copy Markdown
Contributor Author

oddsun commented Apr 27, 2023

Fixed missing empty line by running pre-commit run --all-files.

I realized I can test PR checks by creating PR to forked "main". All checks passed in test PR: oddsun#1.

Maybe this approach could be added to CONTRIBUTING.md, helping decreasing the instances of failed checks.

@oddsun oddsun force-pushed the add-stats-json-cli-arg branch from 0bc1567 to 2901581 Compare April 27, 2023 21:01
Copy link
Copy Markdown
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

I've reviewed this, and spotted a few small things that will need to be fixed.

Comment thread docs/stats.rst Outdated
Comment thread src/memray/reporters/stats.py Outdated
Comment thread src/memray/reporters/stats.py Outdated
Comment thread tests/unit/test_stats_reporter.py Outdated
Comment thread tests/unit/test_stats_reporter.py Outdated
Comment thread tests/unit/test_stats_reporter.py Outdated
Comment thread tests/unit/test_stats_reporter.py Outdated
Comment thread src/memray/reporters/stats.py Outdated
Comment thread src/memray/reporters/stats.py Outdated
@godlygeek
Copy link
Copy Markdown
Contributor

On top of those, can you also update your commit message to use your real name in the signed-off-by? Per https://github.com/bloomberg/memray/blob/main/CONTRIBUTING.md#contribution-licensing we need a real name for the DCO signoff, not a pseudonym.

@oddsun
Copy link
Copy Markdown
Contributor Author

oddsun commented Apr 27, 2023

Got it. Do you want me to make the changes and squash into a single commit again and then update the signoff?

@godlygeek
Copy link
Copy Markdown
Contributor

Got it. Do you want me to make the changes and squash into a single commit again and then update the signoff?

Yes, that would be perfect!

@oddsun oddsun force-pushed the add-stats-json-cli-arg branch 2 times, most recently from 177c34b to 9b127a8 Compare April 27, 2023 23:05
@godlygeek godlygeek force-pushed the add-stats-json-cli-arg branch from 9b127a8 to 92d4267 Compare April 28, 2023 01:38
Copy link
Copy Markdown
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

I've pushed a few more commits here to polish this a little bit: adding some documentation, adding support for --output and --force like our other subcommands that output files accept, and tweaking the format of the JSON a little bit to make it easier to use.

@pablogsal Since I've edited this pretty heavily now, would you mind giving this a review?

@godlygeek godlygeek force-pushed the add-stats-json-cli-arg branch 2 times, most recently from 21baa93 to 051c97c Compare April 28, 2023 01:54
@pablogsal
Copy link
Copy Markdown
Collaborator

I've pushed a few more commits here to polish this a little bit: adding some documentation, adding support for --output and --force like our other subcommands that output files accept, and tweaking the format of the JSON a little bit to make it easier to use.

@pablogsal Since I've edited this pretty heavily now, would you mind giving this a review?

Will review this today or monday 👍

@godlygeek godlygeek force-pushed the add-stats-json-cli-arg branch from 051c97c to 7e69771 Compare April 28, 2023 18:18
@godlygeek
Copy link
Copy Markdown
Contributor

The 3.8 tests are failing because of ipython/ipython#14053 - either the IPython maintainers need to yank the 8.13 release, or we need to land #380 and rebase this.

@sarahmonod
Copy link
Copy Markdown
Contributor

sarahmonod commented May 1, 2023

The 3.8 tests are failing because of ipython/ipython#14053 - either the IPython maintainers need to yank the 8.13 release, or we need to land #380 and rebase this.

I've landed #380, we can always remove that workaround if and when the IPython maintainers yank the 8.13 release.

EDIT: the IPython maintainers have now yanked 8.13 and so #381 reverted the #380 PR ^^

@sarahmonod sarahmonod force-pushed the add-stats-json-cli-arg branch from 7e69771 to b8b3d23 Compare May 1, 2023 13:42
oddsun and others added 4 commits May 1, 2023 17:02
Signed-off-by: Hao Sun <1161465+oddsun@users.noreply.github.com>
- For the histogram, dump objects rather than lists, in order to help
  explain what the values represent.
- For the count of allocations by allocator, dump an object mapping
  allocator name to count.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
@sarahmonod sarahmonod force-pushed the add-stats-json-cli-arg branch from b8b3d23 to 088e2d2 Compare May 1, 2023 21:02
@pablogsal pablogsal merged commit 564a53b into bloomberg:main May 22, 2023
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.

stats with --json output

4 participants