-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
Thanks for your contributions @Zha0q1 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. nice start.
Test cases?
@access2rohit - Can you please help review this PR? |
sure! Zhaoqi has fixed the failing tests now |
Can you fix the failing test ? |
It worked on my computer. Maybe online tests are using a different python version? I will fix this tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Last few minor comments.
As discussed offline, you are planning to update MXNet profiler tutorial in a new PR?
Yeah, that I will do. Thanks for the great advices! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor spelling issue.
debug_str = profiler.dumps(format = 'json') | ||
assert(len(debug_str) > 0) | ||
target_dict = json.loads(debug_str) | ||
print(target_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove print
* add support for sorting and printing aggregate info in json * revert some overwrites * revert to old c apis; added MXAggregateProfileStatsPrintEx instead * style fix * style fixes * more style fixes * rebase * style fixes * fix infinite loop bug * test cases added and bugs fixed * fix test cases * fix testcases * testcases * testcases * testcases * fix doc * use enum to avoid hardcoding * sanity test fix * sanity test fix * add parameter validation * add parameter validation in frontend * validation * removing print() * fix typos
The |
Sure. I will create a pr and add "total_time" soon. |
will do this at my earliest availability after I come back from my trip. Now I don't have access to my laptop (back to US on aug 27, too many family events this week |
Description
This is the work in progress of the changes discussed in #15069. In stead of the original design which is to add a new api, get_summary(), to return the aggregate stats in JSON, I added a few parameters to the existing dumps().
Now dumps() (MXAggregateProfileStatsPrint) supports new parameters: format which controls the return string format, sort_by which specifies by which stat should the entries be sorted, and ascending which specify whether to sort ascendingly.
New api reset() is removed from the original proposal.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes