gh-138122: Replace --interval with --sampling-rate#143085
Merged
pablogsal merged 2 commits intopython:mainfrom Dec 24, 2025
Merged
gh-138122: Replace --interval with --sampling-rate#143085pablogsal merged 2 commits intopython:mainfrom
pablogsal merged 2 commits intopython:mainfrom
Conversation
pablogsal
reviewed
Dec 23, 2025
Doc/library/profiling.sampling.rst
Outdated
| * - Default for ``--interval`` / ``-i`` | ||
| - 100 µs between samples (~10,000 samples/sec) | ||
| * - Default for ``--sampling-rate`` / ``-r`` | ||
| - 10 kHz |
Member
There was a problem hiding this comment.
Please update the documentation to reflect the actual 1 kHz default
pablogsal
reviewed
Dec 23, 2025
| help="sampling interval", | ||
| "-r", | ||
| "--sampling-rate", | ||
| type=_parse_sampling_rate, |
Member
There was a problem hiding this comment.
Nit: The argument is named sampling_rate but after parsing it stores the interval in microseconds, not the rate in Hz. This is kind of confusing in the rest of the code
pablogsal
reviewed
Dec 23, 2025
|
|
||
| match = _RATE_PATTERN.match(rate_str) | ||
| if not match: | ||
| raise argparse.ArgumentTypeError( |
Member
There was a problem hiding this comment.
Nit: Let's add a hint about spaces in the error message since "10 khz" (with space) is rejected but users might try it
pablogsal
reviewed
Dec 23, 2025
Lib/profiling/sampling/cli.py
Outdated
| return process | ||
|
|
||
|
|
||
| _RATE_PATTERN = re.compile(r'^(\d+(?:\.\d+)?)(hz|khz|k)?$', re.IGNORECASE) |
Member
There was a problem hiding this comment.
Suggested change
| _RATE_PATTERN = re.compile(r'^(\d+(?:\.\d+)?)(hz|khz|k)?$', re.IGNORECASE) | |
| _RATE_PATTERN = re.compile(r''' | |
| ^ # Start of string | |
| ( # Group 1: The numeric value | |
| \d+ # One or more digits (integer part) | |
| (?:\.\d+)? # Optional: decimal point followed by digits | |
| ) # Examples: "10", "0.5", "100.25" | |
| ( # Group 2: Optional unit suffix | |
| hz # "hz" - hertz | |
| | khz # "khz" - kilohertz | |
| | k # "k" - shorthand for kilohertz | |
| )? # Suffix is optional (bare number = Hz) | |
| $ # End of string | |
| ''', re.VERBOSE | re.IGNORECASE) |
Member
There was a problem hiding this comment.
Haha you know how much I like to comment these :)
pablogsal
reviewed
Dec 23, 2025
Member
pablogsal
left a comment
pablogsal
left a comment
There was a problem hiding this comment.
LGTM once the documentation is updated! 🚀
Member
|
@lkollar There is a conflict we need to resolve on cli.py |
Sampling rate is more intuitive to the number of samples per second taken, rather than the intervals between samples.
lkollar
force-pushed
the
profiling/sample-rate-cli
branch
from
578e0cc to
f392e82
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Towards #142927.
CC: @pablogsal @ivonastojanovic