ENH: sys.monitoring compliance#327
Conversation
tests/test_complex_case.py::test_varied_complex_invocations()
Broke down test into parametrized subtests for easier identification
of problematic cases
line_profiler/_line_profiler.pyx::LineProfiler
enable(), disable()
Added `sys.monitoring.use_tool_id()` and
`sys.monitoring.free_tool_id()` calls to comply with the
standards post-Python-3.12
line_profiler/_line_profiler.pyx::LineProfiler.{en,dis}able()
- Now only interacting with `sys.monitoring` on the main thread to
avoid compilcations in multithreaded environments
- Now checking for `sys.monitoring` availability at Cython
compilation time, so that the internal methods interacting with
`sys.monitoring` can be substituted with no-ops for Python < 3.12
tests/test_line_profiler.py
test_init()
Fixed flake (extra space)
test_show_func_column_formatting()
New test to ensure that `line_profiler.LineProfiler` is properly
registered to `sys.monitoring` when in use
sys.monitoring compliancesys.monitoring compliance
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
=======================================
Coverage 61.32% 61.32%
=======================================
Files 11 11
Lines 848 848
Branches 186 186
=======================================
Hits 520 520
Misses 274 274
Partials 54 54 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Erotemic
left a comment
There was a problem hiding this comment.
Looks great. Almost everything here is a nitpick. Feel free to discuss or mark them as resolved.
The only thing that I'm going to insist on is going over the test_complex_case case.
- Stripped blank line between imports
- Renamed `h()` -> `get_profiling_tool_name()`
- `test_sys_monitoring()`:
- Added error messages to assertions to be more self-documenting
- Added check that no profiling tool is registered before the profiled
version of `get_profiling_tool_name()` is called
- Dropped these functions and replaced them with C implementations: - `is_main_thread()` - `LineProfiler._sys_monitoring_register()` - `LineProfiler._sys_monitoring_deregister()` - The switch between the post-Python-3.12 implementations and the no-op dummies now happens at Cython compile time, without polluting the namespace with the potentially-unused `_no_op()` method - Added comments to `LineProfiler.enable()` and `.disable()`
Rolled refactoring of `~~::test_varied_complex_invocations()` into a parametrized `pytest` test; see discussion at pyutils#327 (comment)
|
I'm going to change my mind about the C implementation of the threading check after looking at its complexity. It opens way too many potential issues, and I want to land the feature. I doubt having the thread / monitoring check is going to be in the critical path, but in case it is let's make note of this patch in case we want to come back to it. But for now let's just revert to the pure-python calls. My main concern is that we might be missing explicit error checks that need to be done, or GIL management. E.g. it looks like there are missing NULL checks on returns of Apologies for the flip/flop. |
Yikes, I did miss the (EDIT: just realized that since the return values of the C calls are either a
Don't worry about it, it was a fun exercise and I learned quite a bit about the C API and Cython.
In the interest of keeping records, I guess I will fix |
line_profiler/_line_profiler.pyx
_sys_monitoring_register()
Fixed bug where the return value of
`sys.monitoring.use_tool_id()` (`None`) did not have its
refcount decremented
_sys_monitoring_deregister()
Fixed bugs with `sys.monitoring.free_tool_id()`:
- Where if it errored out (shouldn't happen), a Python error
wasn't raised
- Where if it didn't, the return value (`None`) didn't have its
refcount decremented
line_profiler/_line_profiler.pyx::_sys_monitoring_[de]register()
Replaced C implementations with pure Python for maintainability
|
Thanks for the PR! Now that this is merged, please rebase the next one on the latest main. |
Background
This came up during the discussion around #326. Since Python 3.12, profiling tools (like
cProfile.Profile) are supposed to register themselves withsys.monitoring.use_tool_id(sys.monitoring.PROFILER_ID, ...)when in use, andsys.monitoring.free_tool_id(sys.monitoring.PROFILER_ID)when done.This seemed like a trivial addition, but doing so naïvely caused
tests/test_complex_case.py::test_varied_complex_invocations()to fail, presumably due to complications from the test script's use of concurrency. Trying to debug it was difficult, particularly since the singular test was essentially 11 in a trenchcoat.The PR
Hence, this (draft) PR:
tests/test_complex_case.py::test_varied_complex_invocations()into a parametrized test, which allows the 11 subtests to be tested separately.line_profiler/_line_profiler.pyx::LineProfiler.{en,dis}able()thesys.monitoringhooks; when callinguse_tool_id(), the profiler is registered under the'line_profiler'handle.Obviously, the PR is in draft status since the test is still failing.EDIT 22 Mar
The test has been fixed and the PR has been un-drafted with the following changes:
tests/test_line_profiler.py::test_sys_monitoring()to ensure thatLineProfileris interacting withsys.monitoringin the expected manner.CHANGELOG.rstentry.Caveats
Notably, as opposed to
cProfile.Profile,profile.Profiledoesn't register itself withsys.monitoring, and only uses thesys.setprofile()hook (which is intended for profilers written in pure Python). So maybe it isn't strictly necessary for us to go through the trouble...?