gh-138122: Replace --interval with --sampling-rate#143085
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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 |
There was a problem hiding this comment.
Please update the documentation to reflect the actual 1 kHz default
| help="sampling interval", | ||
| "-r", | ||
| "--sampling-rate", | ||
| type=_parse_sampling_rate, |
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
| match=_RATE_PATTERN.match(rate_str) | ||
| ifnotmatch: | ||
| raiseargparse.ArgumentTypeError( |
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
Lib/profiling/sampling/cli.py Outdated
| returnprocess | ||
| _RATE_PATTERN=re.compile(r'^(\d+(?:\.\d+)?)(hz|khz|k)?$', re.IGNORECASE) |
There was a problem hiding this comment.
| _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) |
There was a problem hiding this comment.
Haha you know how much I like to comment these :)
pablogsal left a comment
pablogsal left a comment There was a problem hiding this comment.
LGTM once the documentation is updated! 🚀
pablogsal commented Dec 24, 2025
@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.
lkollarforce-pushed the profiling/sample-rate-cli branch from 578e0cc to f392e82Compare
pablogsal merged commit d4dc3dd into python:mainUh oh!
There was an error while loading. Please reload this page.
2 participants
Towards #142927.
CC: @pablogsal@ivonastojanovic