Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

ruancomelli
Copy link
Contributor

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

Correctly annotate `pstats.FunctionProfile.ncalls` as `str` instead of `int`.
@github-actions

This comment has been minimized.

Copy link
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 :)

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 <[email protected]>
@github-actions

This comment has been minimized.

@ruancomelli
Copy link
Contributor Author

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
Contributor

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

Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants