bpo-29237: Create enum for pstats sorting options #5103

mwidjaja · 2018-01-05T03:52:38Z

https://bugs.python.org/issue29237

ethanfurman

Enum member names need to be in all caps: SortKey.TIME.

Where several values mean the same thing, have one canonical member, and the rest duplicates, e.g.:
CUMULATIVE = 'cumulative'
CUMTIME = 'cumulative'

bedevere-bot · 2018-01-05T04:20:31Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

merwok · 2018-01-05T04:32:21Z

Doc/library/profile.rst

@@ -151,11 +151,17 @@ might try the following sort calls::
   p.sort_stats('name')
   p.print_stats()

+   p.sort_stats(SortKey.name)
+   p.print_stats()


Hello and thanks for the PR!

Here I would recomment keeping only the example with the enum, since it’s the way that we want to recommend.

To match the import style (see import pstats in the previous code block), the example would need to be p.sort_stats(pstats.SortKey.NAME) (assuming the upper-case fix asked by the other reviewer is done).

I wonder if the enum should be defined on the Stats class, to make code shorter: p.sort_stats(p.SortKey.NAME). Maybe worth asking on the ticket to get other people’s opinion.

I would change the import to

from pstats import Stats, SortKey

The pstats module is all about stats processing, so there is no need to hide SortKey inside the Stats class.

Thank you for the comment. Re: 'Here I would recomment keeping only the example with the enum, since it’s the way that we want to recommend.' I could do that, but I have also received comment about having both example as the string argument is still valid. Showing only the enum would make the doc incomplete.

Yes there is a need to also show an example with a string form, but not all code examples need both forms.

@mwidjaja

When you make the duplicate SortKeys actually duplicates, please put the better name first -- something like:

class SortKey(str, Enum): calls = 'calls' ncalls = 'calls' cumulative = 'cumulative' cumtime = 'cumulative' module = 'module' file = 'module' filename = 'module' line = 'line' name = 'name' nfl = 'nfl' pcalls = 'pcalls' stdname = 'stdname' time = 'time' tottime = 'time'

mwidjaja · 2018-01-05T17:40:11Z

@ethanfurman:

Thank you for your review. Regarding the request to change CUMTIME to 'cumulative', wouldn't this break the existing code that uses 'cumtime' as the sort criteria? I guess I can internally map the string 'cumtime' to 'cumulative' to allow it. If you have other suggestion, let me know.

ethanfurman · 2018-01-05T19:24:38Z

On 01/05/2018 09:40 AM, mwidjaja wrote: Thank you for your review. Regarding the request to change CUMTIME to 'cumulative', wouldn't this break the existing code that uses 'cumtime' as the sort criteria? I guess I can internally map the string 'cumtime' to 'cumulative' to allow it. If you have other suggestion, let me know.

There is a separate code path for the string arguments, so any string-based code would continue to work.

mwidjaja · 2018-01-06T00:52:49Z

@ethanfurman:
I do not see that path. Would you point me to the string-based code path?
I see this path for both arg type:

for word in field:
    sort_tuple = sort_tuple + sort_arg_defs[word][0]
    self.sort_type += connector + sort_arg_defs[word][1]
    connector = ", "

merwok · 2018-01-08T00:22:41Z

Doc/library/profile.rst

   p.print_stats()

 The first call will actually sort the list by function name, and the second call
 will print out the statistics.  The following are some interesting calls to
 experiment with::

   p.sort_stats('cumulative').print_stats(10)
+   p.sort_stats(SortKey.cumulative).print_stats(10)


I still think showing both ways in these examples is not useful and could create confusion.
It would be enough to have one example here and the changes that you already did in the method docs later in this file.

Ok. unless there're objections from anyone else, I'll remove them and just keep the one in the method doc.

mwidjaja · 2018-01-10T18:18:25Z

@ethanfurman, would you be able to give me some hints on your comments about separate code path. Thank you.

mwidjaja · 2018-01-13T20:54:01Z

I have made the requested changes; please review again.

bedevere-bot · 2018-01-13T20:54:03Z

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

mwidjaja · 2018-01-17T17:24:19Z

@merwok, @ethanfurman : If there's anything else I need to do, let me know, otherwise this PR is ready for another review. Thank you

merwok · 2018-01-17T18:54:52Z

Doc/library/profile.rst

@@ -148,14 +148,14 @@ entries according to the standard module/line/name string that is printed. The
 :meth:`~pstats.Stats.print_stats` method printed out all the statistics.  You
 might try the following sort calls::

-   p.sort_stats('name')
+   p.sort_stats(SortKey.name)


If people follow the code examples here, SortKey will be undefined. It needs to be imported.

merwok · 2018-01-17T18:55:24Z

Doc/library/profile.rst

@@ -424,6 +428,8 @@ Analysis of the profiler data is done using the :class:`~pstats.Stats` class.

      .. For compatibility with the old profiler.

+      .. versionadded:: 3.7
+         Added the SortKey enums.


ethanfurman · 2018-01-17T19:06:53Z

Lib/pstats.py

-              "pcalls"    : (((0,-1),              ), "primitive call count"),
-              "stdname"   : (((7, 1),              ), "standard name"),
-              "time"      : (((2,-1),              ), "internal time"),
-              "tottime"   : (((2,-1),              ), "internal time"),


Since we still have to support string arguments, and the SortKey members are also strings, I would just leave the original code alone here.

ethanfurman · 2018-01-17T19:20:12Z

Lib/pstats.py

+    STDNAME = 'stdname'
+    TIME = 'time'
+    TOTTIME = 'tottime'
+


My initial understanding of this was flawed. Since we need to support the old string values we can't just have CUMTIME and CUMULATIVE map to "cumulative" because then "cumtime" is gone (unless we do extra work, which I see you did).

What we need here is a MultiValueEnum, so SortKey.CUMULATIVE maps to both "cumulative" and "cumtime".

The Enum should look like this:

class SortKey(str, Enum): CALLS = 'calls', 'ncalls' CUMULATIVE = 'cumulative', "cumtime" MODULE = 'module', 'filename', 'file' LINE = 'line' NAME = 'name' NFL = 'nfl' PCALLS = 'pcalls' STDNAME = 'stdname' TIME = 'time', 'tottime' def __new__(cls, *values): obj = object.__new__(cls) # first value is canonical value obj._value_ = values[0] for other_value in values[1:]: cls._value2member_map_[other_value] = obj obj._all_values = values return obj

Operations like SortKey('file') will return SortKey.MODULE. The downside is that SortKey.FILE is undefined, but I think that is an acceptable trade-off since we are still supporting the original string values, and those values can be used to get a correct SortKey member.

ethanfurman · 2018-01-17T19:21:20Z

Lib/pstats.py

+        else:
+            for word in field:
+                if isinstance(word, str) and word == 'cumtime':
+                    word = 'cumulative'


With the new SortKey enum, these three lines are no longer needed.

ethanfurman · 2018-01-17T19:22:49Z

Lib/test/test_pstats.py

+
+    def test_sort_stats_string(self):
+        for arg in SortKey:
+            arg = arg.value


When dealing with Enums, member is a better name than arg.

ethanfurman · 2018-01-17T19:24:24Z

Lib/test/test_pstats.py

+            # 'file' sorting criteria will not work because it creates
+            # ambiquity with 'filename'
+            if arg == 'file':
+                continue


I don't understand -- 'file', 'filename', and 'module' should all create the same sort -- what ambiguity is there?

While what you're saying is essentially correct, however as it is currently implemented (without any of my changes), we're allowed to abbreviate the string argument into the sort_stats as long as the abbreviation is unambiguous. For example, sort_stats would work if either 'filename' or 'filena' string is passed in. But, if string 'file' is passed, then sort_stats can't differentiate between 'filename' or 'file' and sort_stats will fail with KeyError exception. We should probably remove 'file'. I can do that, if you agree. I'm guessing that no existing code is using 'file' argument at this time.

file and filename map to the same stat, so if we removed file and somebody passed in file it should still work.

Remove "file" and write a test to make sure it still works as intended.

Ok. Will make another pass to remove "file".

ethanfurman · 2018-01-17T19:29:26Z

Lib/test/test_pstats.py

+                             self.stats.sort_arg_dict_default[arg_str][-1])
+
+    def test_sort_stats_string(self):
+        for arg in SortKey:


Since this is the string test, we should be iterating through all the possible strings (no longer a given with the new SortKey enum). Listing them all is probably the best way to go:

for arg in ('calls', 'ncalls', 'cumtime', 'cumulative', ... ):

ethanfurman

Some changes with regards to how the SortKey enum is created, and the cascading changes from that.

My apologies for my initial misunderstanding of the problem.

bedevere-bot · 2018-01-17T19:31:17Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-bot · 2018-01-25T00:31:46Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mwidjaja · 2018-01-25T16:30:33Z

I have made the requested changes; please review again.

bedevere-bot · 2018-01-25T16:30:36Z

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

ethanfurman · 2018-01-25T16:40:12Z

Doc/library/profile.rst

@@ -393,7 +393,7 @@ Analysis of the profiler data is done using the :class:`~pstats.Stats` class.
      +------------------+---------------------+----------------------+
      | ``'filename'``   | N/A                 | file name            |
      +------------------+---------------------+----------------------+
-      | ``'module'``     | SortKey.MODULE      | file name            |
+      | ``'module'``     | SortKey.FILENAME    | file name            |


Move SortKey.FILENAME up one line to the 'filename' entry, change the 'module' entry to N/A, and we're done! Thank you!

bedevere-bot · 2018-01-25T16:41:37Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mwidjaja · 2018-01-26T04:12:38Z

Thank you @ethanfurman @merwok for the review comments.

I have made the requested changes; please review again.

bedevere-bot · 2018-01-26T04:12:41Z

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

bedevere-bot added the awaiting review label Jan 5, 2018

the-knights-who-say-ni added the CLA signed label Jan 5, 2018

ethanfurman requested changes Jan 5, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 5, 2018

merwok reviewed Jan 5, 2018

View reviewed changes

mwidjaja force-pushed the fix-bpo-29237 branch from 91d8117 to b098e7d Compare January 7, 2018 05:38

merwok reviewed Jan 8, 2018

View reviewed changes

mwidjaja force-pushed the fix-bpo-29237 branch from 2a79106 to 9413145 Compare January 13, 2018 20:35

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 13, 2018

merwok reviewed Jan 17, 2018

View reviewed changes

ethanfurman reviewed Jan 17, 2018

View reviewed changes

ethanfurman requested changes Jan 17, 2018

View reviewed changes

bedevere-bot removed the awaiting change review label Jan 17, 2018

bedevere-bot removed the awaiting change review label Jan 25, 2018

bedevere-bot added the awaiting changes label Jan 25, 2018

mwidjaja force-pushed the fix-bpo-29237 branch from 4e26d4f to 385b383 Compare January 25, 2018 16:00

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 25, 2018

ethanfurman requested changes Jan 25, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 25, 2018

mwidjaja added 9 commits January 25, 2018 22:49

bpo-29237: Create enum for pstats sorting options

c28feaa

Change enums member to uppercase and other review comments

5553944

Fix failing pstats.py unittest

125bfda

Further Doc updates/changes

d846ba7

Incorporate review comments

9283cb7

Whitespace fixes

259e0f9

Remove 'file' argument

6adc715

Change SortKey.MODULE to SortKey.FILENAME

ef409c2

Minor doc update

ad7a395

mwidjaja force-pushed the fix-bpo-29237 branch from 385b383 to ad7a395 Compare January 26, 2018 03:49

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 26, 2018

ethanfurman merged commit 863b1e4 into python:master Jan 26, 2018

bedevere-bot removed the awaiting change review label Jan 26, 2018

ethanfurman self-assigned this Jan 26, 2018

mwidjaja deleted the fix-bpo-29237 branch January 26, 2018 13:34

bpo-29237: Create enum for pstats sorting options #5103

bpo-29237: Create enum for pstats sorting options #5103

Conversation

mwidjaja commented Jan 5, 2018 • edited by bedevere-bot Loading

ethanfurman left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 5, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwidjaja commented Jan 5, 2018

ethanfurman commented Jan 5, 2018 via email • edited Loading

mwidjaja commented Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwidjaja commented Jan 10, 2018

mwidjaja commented Jan 13, 2018

bedevere-bot commented Jan 13, 2018

mwidjaja commented Jan 17, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfurman left a comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 17, 2018

bedevere-bot commented Jan 25, 2018

mwidjaja commented Jan 25, 2018

bedevere-bot commented Jan 25, 2018

Choose a reason for hiding this comment

bedevere-bot commented Jan 25, 2018

mwidjaja commented Jan 26, 2018

bedevere-bot commented Jan 26, 2018

mwidjaja commented Jan 5, 2018 •

edited by bedevere-bot

Loading

ethanfurman commented Jan 5, 2018 via email •

edited

Loading

mwidjaja commented Jan 6, 2018 •

edited

Loading