Skip to content

fix: annotate pstats.FunctionProfile.ncalls as str#8712

Merged
AlexWaygood merged 3 commits intopython:masterfrom
ruancomelli:ruancomellipstats-FunctionProfile-ncalls
Sep 10, 2022
Merged

fix: annotate pstats.FunctionProfile.ncalls as str#8712
AlexWaygood merged 3 commits intopython:masterfrom
ruancomelli:ruancomellipstats-FunctionProfile-ncalls

Conversation

@ruancomelli
Copy link
Copy Markdown
Contributor

@ruancomelli ruancomelli commented Sep 8, 2022

Correctly annotate pstats.FunctionProfile.ncalls as str instead of int, aligning the static type with the runtime one.

The reason why it is a string at runtime is indicated in the profiling instant user's manual:

When there are two numbers in the first column (for example 3/1), it means that the function recursed. The second value is the number of primitive calls and the former is the total number of calls. Note that when the function does not recurse, these two values are the same, and only the single figure is printed.

This behavior can be tested at runtime with the following script:

from cProfile import Profile
from pstats import Stats

# execute random code to generate profile stats
with Profile() as profile:
    print("foo")

# get a sample function profile
stats_profile = Stats(profile).get_stats_profile()
function_profile = next(iter(stats_profile.func_profiles.values()))

# test that the type of `function_profile.ncalls` is indeed `str`
assert type(function_profile.ncalls) is str

@ruancomelli
fix: annotate pstats.FunctionProfile.ncalls as str
8e0a376 
Correctly annotate `pstats.FunctionProfile.ncalls` as `str` instead of `int`.
@github-actions

This comment has been minimized.

Sign in to view
AlexWaygood
AlexWaygood reviewed Sep 9, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Really interesting. I verified this both by looking at the code in CPython and by trying out your test script -- this seems to be correct.

I'm guessing what happened here is we just copied over the annotation from the CPython source, which appears to be completely incorrect. (Or maybe it's actually correct, and there's meant to be a call to int() somewhere, but that never happens?)

Either way, this is quite strange, so let's add a comment. And you could consider opening a CPython issue/PR about it :)

@ruancomelli ruancomelli mentioned this pull request Sep 10, 2022
Merged
@ruancomelli @AlexWaygood
Add comment pointing out the difference to CPython
37e3afc 
Since this change introduces a difference between typeshed and CPython, add a comment pointing that out and linking to this PR.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@github-actions

This comment has been minimized.

Sign in to view
@ruancomelli
Copy link
Copy Markdown
Contributor Author

ruancomelli commented Sep 10, 2022

Thank you for your review @AlexWaygood!

Also thanks for your suggestion, I opened a PR to fix this in CPython as well: python/cpython#96741.

I'm guessing what happened here is we just copied over the annotation from the CPython source, which appears to be completely incorrect. (Or maybe it's actually correct, and there's meant to be a call to int() somewhere, but that never happens?)

Just to answer your last comment, it is intentional that ncalls is a string. See the line where it is defined: https://github.com/python/cpython/blob/6281affee6423296893b509cd78dc563ca58b196/Lib/pstats.py#L372
This is because there is a differentiation between "primitive number of calls" and "total number of calls". If they are equal, only one number is shown (this is the value on the left, str(nc)) - but, if they differ, both are shown, separated by a slash (that is str(nc) + '/' + str(cc)).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 10, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

AlexWaygood
AlexWaygood approved these changes Sep 10, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks @ruancomelli!

@AlexWaygood AlexWaygood merged commit 04a88d5 into python:master Sep 10, 2022
@ruancomelli ruancomelli deleted the ruancomellipstats-FunctionProfile-ncalls branch September 10, 2022 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood AlexWaygood approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ruancomelli @AlexWaygood