GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-138122: Replace --interval with --sampling-rate by lkollar · Pull Request #143085 · python/cpython

Conversation

lkollar commented Dec 22, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Towards #142927.

CC: @pablogsal @ivonastojanovic

Issue: Implement PEP 799 – A dedicated profiling package for organizing Python profiling tools #138122

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

lkollar requested a review from pablogsal as a code owner

December 22, 2025 21:09

bedevere-app Bot mentioned this pull request

Dec 22, 2025

Implement PEP 799 – A dedicated profiling package for organizing Python profiling tools #138122

Closed

bedevere-app Bot added the awaiting review label

Dec 22, 2025

pablogsal reviewed

Dec 23, 2025

View reviewed changes

Comment thread

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

pablogsal Dec 23, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please update the documentation to reflect the actual 1 kHz default

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal reviewed

Dec 23, 2025

View reviewed changes

Comment thread

Lib/profiling/sampling/cli.py

		help="sampling interval",
		"-r",
		"--sampling-rate",
		type=_parse_sampling_rate,

pablogsal Dec 23, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal reviewed

Dec 23, 2025

View reviewed changes

Comment thread

Lib/profiling/sampling/cli.py


		match = _RATE_PATTERN.match(rate_str)
		if not match:
		raise argparse.ArgumentTypeError(

pablogsal Dec 23, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: Let's add a hint about spaces in the error message since "10 khz" (with space) is rejected but users might try it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal reviewed

Dec 23, 2025

View reviewed changes

Comment thread

Lib/profiling/sampling/cli.py Outdated

		return process


		_RATE_PATTERN = re.compile(r'^(\d+(?:\.\d+)?)(hz\|khz\|k)?$', re.IGNORECASE)

pablogsal Dec 23, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal Dec 23, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Haha you know how much I like to comment these :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal reviewed

Dec 23, 2025

View reviewed changes

pablogsal left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM once the documentation is updated! 🚀

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

pablogsal added the skip news label

Dec 24, 2025

pablogsal commented Dec 24, 2025

Copy link

Copy Markdown

Member

@lkollar There is a conflict we need to resolve on cli.py

lkollar reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lkollar added 2 commits

December 24, 2025 13:14


          Standardise time units

a13a4aa


          Replace --interval with --sampling-rate

f392e82

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

December 24, 2025 13:21

pablogsal approved these changes

Dec 24, 2025

View reviewed changes

pablogsal left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM! Great work 👌

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lkollar reacted with heart emoji

All reactions

❤️ 1 reaction

bedevere-app Bot added awaiting merge and removed awaiting review labels

Dec 24, 2025

pablogsal enabled auto-merge (squash)

December 24, 2025 13:23

pablogsal merged commit d4dc3dd into python:main

Dec 24, 2025

50 checks passed

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the awaiting merge label

Dec 24, 2025

Reviewers

pablogsal

pablogsal approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

lkollar commented Dec 22, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

pablogsal Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

pablogsal Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

pablogsal Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Dec 24, 2025

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lkollar commented Dec 22, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

pablogsal Dec 23, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.