Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-142654: show the clear error message when sampling on an unknown PID#142655
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
kemingy commented Dec 13, 2025 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
…nown PID Signed-off-by: Keming <kemingy94@gmail.com>
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.
Pull request overview
This PR improves error handling when attempting to profile a process with an invalid PID by simplifying the error message. Instead of showing a confusing stack trace with multiple exceptions, it now displays only the final, clear error message about failing to find the PyRuntime section.
Key Changes:
- Wraps
RemoteUnwinderinitialization in a try-except block that converts exceptions toSystemExit - Updates test expectations from
OSError/RuntimeErrortoSystemExit - Adds a NEWS entry documenting the improvement
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
Lib/profiling/sampling/sample.py | Adds exception handling in SampleProfiler.__init__ to convert errors to SystemExit for cleaner user-facing messages |
Lib/test/test_profiling/test_sampling_profiler/test_integration.py | Updates test to expect SystemExit instead of OSError/RuntimeError for invalid PIDs |
Misc/NEWS.d/next/Library/2025-12-13-10-34-59.gh-issue-142654.fmm974.rst | Documents the error message improvement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Lib/test/test_profiling/test_sampling_profiler/test_integration.py Outdated Show resolvedHide resolved
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Keming <kemingy94@gmail.com>
Signed-off-by: Keming <kemingy94@gmail.com>
pablogsal commented Dec 14, 2025
Signed-off-by: Keming <kemingy94@gmail.com>
Signed-off-by: Keming <kemingy94@gmail.com>
Signed-off-by: Keming <kemingy94@gmail.com>
kemingy commented Dec 15, 2025
Hi @pablogsal I have added the process check. Please take a look. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Lib/profiling/sampling/cli.py Outdated
| def_handle_attach(args): | ||
| """Handle the 'attach' command.""" | ||
| ifnot_is_process_running(args.pid): | ||
| raisesys.exit(f"Process with PID {args.pid} is not running.") |
pablogsalDec 15, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
I would prefer to raise some custom exception here and catch it in __main__ like the other exceptions. Perhaps we should also do the same for the other usages of sys.exit.
Do you mind creating a errors.py file with custom exceptions for the different case, raise them and properly handle the messages in __main__?
Uh oh!
There was an error while loading. Please reload this page.
pablogsal commented Dec 15, 2025
We also need a test for this |
Signed-off-by: Keming <kemingy94@gmail.com>
kemingy commented Dec 16, 2025
Hi @pablogsal Thanks for your advice. I have added the |
Signed-off-by: Keming <kemingy94@gmail.com>
d4095f2 into python:mainUh oh!
There was an error while loading. Please reload this page.
pablogsal commented Dec 17, 2025
Made some small fixes but great job @kemingy! Thanks for the PR 🚀 |
Before this change:
After this change:
This aligns the behavior with attaching to a process that doesn't have the PyRuntime section.