Skip to content

Conversation

@lkollar
Copy link
Contributor

@lkollarlkollar commented Jul 19, 2025

Add -m and filename arguments to the sampling profiler to launch the specified Python program in a subprocess and start profiling it. Previously only a PID was accepted, this can now be done by passing -p PID.

@lkollarlkollarforce-pushed the sample-module branch 2 times, most recently from 5a351da to 1bf048aCompareJuly 19, 2025 14:07
@lkollarlkollar marked this pull request as ready for review July 19, 2025 14:59
@pablogsalpablogsal added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Jul 29, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 1bf048a 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136777%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 29, 2025
@pablogsalpablogsal requested a review from CopilotJuly 29, 2025 16:20
Copy link

CopilotAI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the sampling profiler to support profiling modules and scripts by launching them in subprocesses, in addition to the existing PID-based profiling. The change improves the usability of the profiler by allowing users to profile Python programs from startup rather than only attaching to existing processes.

  • Adds -m and script filename arguments as mutually exclusive alternatives to the existing -p PID option
  • Updates argument parsing to handle the new target modes with proper validation
  • Implements subprocess management with graceful termination handling

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

FileDescription
Lib/profile/sample.pyAdds argument parsing for module and script modes, implements subprocess launching and management
Lib/test/test_sample_profiler.pyUpdates existing CLI tests to use -p flag and adds comprehensive test coverage for new module/script functionality
Comments suppressed due to low confidence (1)

Lib/test/test_sample_profiler.py:1637

  • The test uses contextlib.chdir() to change directories for module discovery, but doesn't test the case where the module is not found or the directory change fails. Consider adding a test case for module resolution errors.
 contextlib.chdir(tempdir.name), 

Comment on lines 736 to 739
ifnot(args.pidorargs.moduleorargs.script):
parser.error(
"You must specify either a process ID (-p), a module (-m), or a script to run."
)
Copy link

CopilotAIJul 29, 2025

Choose a reason for hiding this comment

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

The condition not(args.pid or args.module or args.script) is redundant since the mutually exclusive group is already marked as required=True. The argparse library will automatically enforce that one of these options is provided.

Suggested change
ifnot(args.pidorargs.moduleorargs.script):
parser.error(
"You must specify either a process ID (-p), a module (-m), or a script to run."
)
# The mutually exclusive group already enforces that one of these arguments is required.

Copilot uses AI. Check for mistakes.
process=subprocess.Popen(cmd)

try:
exit_code=process.wait(timeout=0.1)
Copy link

CopilotAIJul 29, 2025

Choose a reason for hiding this comment

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

The hardcoded timeout of 0.1 seconds is a magic number. Consider defining this as a named constant or making it configurable, as some programs may have longer startup times.

Suggested change
exit_code=process.wait(timeout=0.1)
exit_code=process.wait(timeout=DEFAULT_PROCESS_WAIT_TIMEOUT)

Copilot uses AI. Check for mistakes.
ifprocess.poll() isNone:
process.terminate()
try:
process.wait(timeout=2)

This comment was marked as outdated.

@AA-TurnerAA-Turner changed the title [gh-135953] Profile a module or script with sampling profilergh-135953: Profile a module or script with sampling profilerAug 4, 2025
@lkollar
Copy link
ContributorAuthor

Thanks @AA-Turner, I think I addressed all your comments.

lkollar added 11 commits August 6, 2025 14:26
Add `-m` and `filename` arguments to the sampling profiler to launch the specified Python program in a subprocess and start profiling it. Previously only a PID was accepted, this can now be done by passing `-p PID`.
These args are already mutually exclusive, but we need to check if at least on module argument has been passed.
In this case the subprocess will go into zombie state until we can poll it. We can simply assume this is the case if it's still detected as running when we get a ValueError.
Improve the return value check to be able to raise a ProcessLookupError when the remote process is not available. Mach uses composite error values where higher error values indicate specific subsystems. We can use the err_get_code function to mask the higher bits to make our error checking more robust in case the subsystem bits are set. For example, in some situations if the process is in zombie state, we can get KERN_NO_SPACE (0x3) but the actual return value is 0x10000003 which indicates a specific subsystem, thus we need to use err_get_code to extract the error value. This also improves how KERN_INVALID_ARGUMENT is handled to check whether we got a generic invalid argument error, or if the process is no longer accessible.
@pablogsalpablogsal requested review from 1st1 and ambv as code ownersAugust 8, 2025 12:34
@pablogsalpablogsalforce-pushed the sample-module branch 3 times, most recently from 758af04 to 289d868CompareAugust 8, 2025 13:31
@pablogsalpablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 8, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 0338ee1 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136777%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 8, 2025
@pablogsal
Copy link
Member

@lkollar Seems test_sample_target_module has a race since is failin on some buildbots:

AssertionError: 'slow_fibonacci' not found in 'Captured 10001 samples in 1.00 seconds Sample rate: 10000.94 samples/sec Error rate: 99.10% Profile Stats: nsamples sample% tottime (ms) cumul% cumtime (ms) filename:lineno(function) 90/90 100.0 9.000 100.0 9.000 sample.py:96(SampleProfiler.sample) 0/90 0.0 0.000 100.0 9.000 sample.py:549(sample) 0/90 0.0 0.000 100.0 9.000 sample.py:590(wait_for_process_and_sample) 0/90 0.0 0.000 100.0 9.000 sample.py:790(main) 0/90 0.0 0.000 100.0 9.000 test_sample_profiler.py:1640(TestSampleProfilerIntegration.test_sample_target_module) 0/90 0.0 0.000 100.0 9.000 case.py:613(TestCase._callTestMethod) 0/90 0.0 0.000 100.0 9.000 case.py:667(TestCase.run) Legend: nsamples: Direct/Cumulative samples (direct executing / on call stack) sample%: Percentage of total samples this function was directly executing tottime: Estimated total time spent directly in this function cumul%: Percentage of total samples when this function was on the call stack cumtime: Estimated cumulative time (including time in called functions) filename:lineno(function): Function location and name Summary of Interesting Functions: Functions with Highest Direct/Cumulative Ratio (Hot Spots): 1.000 direct/cumulative ratio, 100.0% direct samples: sample.py:(SampleProfiler.sample) Functions with Highest Call Frequency (Indirect Calls): 90 indirect calls, 100.0% total stack presence: sample.py:(sample) 90 indirect calls, 100.0% total stack presence: sample.py:(wait_for_process_and_sample) 90 indirect calls, 100.0% total stack presence: sample.py:(main) Functions with Highest Call Magnification (Cumulative/Direct): 

@pablogsal
Copy link
Member

pablogsal commented Aug 9, 2025

why is the sample.py code in the results? Also somehow we have Error rate: 99.10%????

@pablogsal
Copy link
Member

Oh, I think this is because we are sampling before the other process calls execv so we see the forked one! We may need some sync mechanism between the process and the profilee.

@pablogsalpablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 9, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 3847fff 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136777%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 9, 2025
@pablogsalpablogsal merged commit 4497ad4 into python:mainAug 11, 2025
129 checks passed
maurycy added a commit to maurycy/cpython that referenced this pull request Aug 12, 2025
* main: pythongh-137288: Update 3.14 magic numbers (pythonGH-137665) pythongh-135228: When @DataClass(slots=True) replaces a dataclass, make the original class collectible (take 2) (pythonGH-137047) pythongh-126008: Improve docstrings for Tkinter cget and configure methods (pythonGH-133303) pythongh-131885: Use positional-only markers for ``max()`` and ``min()`` (python#131868) pythonGH-137426: Remove code deprecation of `importlib.abc.ResourceLoader` (pythonGH-137567) pythongh-125897: Mark range function parameters as positional only (python#125945) pythongh-137400: Fix a crash when disabling profiling across all threads (pythongh-137471) pythongh-115766: Fix IPv4Interface.is_unspecified (pythonGH-137326) pythongh-128813: cleanup C-API docs for PyComplexObject (pythonGH-137579) pythongh-135953: Profile a module or script with sampling profiler (python#136777) Fix documentation of hash in PyHash_FuncDef (python#137595)
@lkollarlkollar deleted the sample-module branch August 13, 2025 18:21
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@lkollar@bedevere-bot@pablogsal@AA-Turner