Skip to content

gh-111277: In summarize_stats.py, don't fail fast on invalid ratios #111278

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 31, 2023
Merged

gh-111277: In summarize_stats.py, don't fail fast on invalid ratios #111278

merged 2 commits into from
Oct 31, 2023

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Oct 24, 2023 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Oct 24, 2023
Closed
@mdboom mdboom requested a review from markshannon October 24, 2023 17:10
@markshannon
Copy link
Member

Don't we want to fail if the denominator is zero? That seems like a logic bug to me.

gvanrossum
gvanrossum approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I'll merge. Do you need this backported?

@gvanrossum
Copy link
Member

Actually let me wait until you have answered Mark's question about den == 0 being an error.

@mdboom
Copy link
Contributor Author

mdboom commented Oct 30, 2023

Don't we want to fail if the denominator is zero? That seems like a logic bug to me.

I thought so too, but the tricky bit is that logic bug, if it happens, happens way over during stats collection at Python runtime. If we fail hard here, we don't get any stats out at all. With this change, we know there is an error (the !!), but we can at least see everything else and attempt to fix the error within CPython.

@gvanrossum gvanrossum merged commit 9495bca into python:main Oct 31, 2023
FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
@mdboom @FullteaR
pythongh-111277: In summarize_stats.py, don't fail fast on invalid ra…
a967896 
…tios (python#111278)
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@mdboom @aisk
pythongh-111277: In summarize_stats.py, don't fail fast on invalid ra…
13fba32 
…tios (python#111278)
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@mdboom @Glyphack
pythongh-111277: In summarize_stats.py, don't fail fast on invalid ra…
2c8d206 
…tios (python#111278)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

@markshannon markshannon Awaiting requested review from markshannon

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mdboom @markshannon @gvanrossum