ENH: more intuitive profiling-target selection #337
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
==========================================
+ Coverage 65.33% 69.33% +4.00%
==========================================
Files 13 18 +5
Lines 1073 1611 +538
Branches 234 341 +107
==========================================
+ Hits 701 1117 +416
- Misses 310 416 +106
- Partials 62 78 +16
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
65b1f46 to
c33d62b
Compare
|
Haven't looked at this yet. Busy crunching for a paper deadline. But I did come across a use-case where I attempted to demo auto-profiling but it came up with an error. I would expect that However, I tested: |
|
It seems that global enum is indeed problematic, but the problem lies deeper than >>> import runpy
>>> runpy.run_module('calendar', {}, '__main__')
Traceback (most recent call last):
File "<python-input-1>", line 1, in <module>
runpy.run_module('calendar', {}, '__main__')
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<frozen runpy>", line 229, in run_module
File "<frozen runpy>", line 88, in _run_code
File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 813, in <module>
main()
~~~~^^
File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 800, in main
result = cal.formatyear(datetime.date.today().year, **optdict)
File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 420, in formatyear
for (i, row) in enumerate(self.yeardays2calendar(theyear, m)):
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 317, in yeardays2calendar
months = [self.monthdays2calendar(year, m) for m in Month]
~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^
File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 289, in monthdays2calendar
days = list(self.itermonthdays2(year, month))
<
8000
span class="pl-v">File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 245, in itermonthdays2
for i, d in enumerate(self.itermonthdays(year, month), self.firstweekday):
~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 233, in itermonthdays
day1, ndays = monthrange(year, month)
~~~~~~~~~~^^^^^^^^^^^^^
File "/opt/homebrew/Cellar/python@3.13/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 172, in monthrange
ndays = mdays[month] + (month == FEBRUARY and isleap(year))
^^^^^^^^
NameError: name 'FEBRUARY' is not definedSince The problem seems to be that Calling |
|
Conflicts need to be fixed here. I'm also not sure of adding both |
|
Fair enough, since the eager behavior is the more intuitive one; will do. |
|
Dunno why but the CI tests seem substantially more sluggish on newer Pythons:
It's even more egregious on my own machine:
#327 might be partially to blame since that's extra stuff (though menial) that Python 3.12+ has to do,1 but it couldn't have been the only reason since 3.10 was also slow... Footnotes
|
|
Hmm, I ran ./run_tests.py twice on main on my local venvs. I got:
So, my 3.10 tests were faster than yours. We should add some performance regression tests, but I'm not supper worried about a 30 second CI runtime. I deal with projects that have 20 minute-per-run CI times. If only there was some sort of profiling tool that we could use to test which lines are slower between versions... :) |
|
Making I did try using my own plugin (https://gitlab.com/TTsangSC/pytest-autoprofile) to profile the test suite, but there are two major bottlenecks:
Maybe I can try to do as
EDIT: a possible solution to the multiple-profiler problem may be that we make the C-level profiler a singleton responsible for all the actual profiling and tracing in the process, and have
And thanks for the data – I've had the suspicion that it's my machine that's acting odd since we didn't have that kind of slowdown in CI... really hoping that I won't have to do anything more involved than reinstalling my Pythons to fix that. |
| @@ -605,6 +738,9 @@ def _main(options, module=False): | |||
| print(f'{py_exe} -m pstats "{options.outfile}"') | |||
| else: | |||
| print(f'{py_exe} -m line_profiler -rmt "{options.outfile}"') | |||
| # Fully disable the profiler | |||
There was a problem hiding this comment.
Some tests like tests/test_kernprof.py:: test_kernprof_sys_restoration() (and a good chunk of tests/test_line_profiler.py) are in-process, and for that it's best that kernprof.py::main() cleans up after itself, or we'll get failures like https://github.com/pyutils/line_profiler/actions/runs/15092575370/job/42423114894 (because the previous profiler instance isn't disabled and still has the sys.monitoring lock).
One can argue that I should've wrote said test so that it instead handles the cleanup, but since other components in kernprof.py have been imported in other tests, I'd say it's the best to treat kernprof as a module (instead of a mere script) and minimize the side effects that its public functions has upon being called.
There was a problem hiding this comment.
Ah, that makes a lot of sense. I agree with minimizing side effects and looking at kernprof as a module.
line_profiler/line_profiler.py
Outdated
| def add_module(self, mod): | ||
| """ Add all the functions in a module and its classes. | ||
| def _add_namespace(self, duplicate_tracker, namespace, *, | ||
| match_scope='none', wrap=False): |
There was a problem hiding this comment.
Small comment here about how this is used to recursively iterate through members of a containers and wrap profile-able objects according to match-scope .
As a nitpick, I think the duplicate_tracker should be a keyword only argument that defaults to None, and is initialized to a set if it is None. Then we can get grid of the cryptic empty set instances created in the add_class and add_module functions. I would also recommend calling it "seen", and just using "seen.add" instead of assigning a variable to its add method. That just feels more idiomatic and obvious to me.
Could probably do the same with add_func and wrap_func, as they are only used once. It won't make the lines too long and actually has a minor positive impact on performance as you don't do an attribute access if you don't need it.
There was a problem hiding this comment.
Fair enough, the idea was to pre-fetch commonly-used callables into the namespace to avoid attribute access in a tight-ish loop, but then again the two shouldn't really cost that differently unless we have some .__getattribute__() magic (which we don't).
line_profiler/line_profil
A3E2
er.py
Outdated
| 'none']): | ||
| Whether (and how) to match the scope of member classes | ||
| and decide on whether to add them: | ||
| - 'exact': only add classes defined locally in this |
There was a problem hiding this comment.
I'm wondering if there is a better name for match_scope. I think a name ending in "_policy" might make it more clear that the right choice for this might depend on context.
I think I have a grasp on what it's doing but an example might helpful, and when you might want to change the policy.
Is it user-facing at all, I only see it used in line_profiler.autoprofile.line_profiler_utils.add_imported_function_or_module, and I don't see how a user could change it. Is this added mostly in case we want to modify behavior later? It's adding quite a bit of complexity for something that seems to always take the value "siblings".
It also might be cleaner and more extensible to define it as a StrEnum, which requires Python 3.11, but we can add a backport in a utils module:
class StrEnum(str, Enum):
"""
Minimal string enum that works with Python 3.6+
"""
def __str__(self):
return self.value
@classmethod
def _missing_(cls, value):
# Allow case-insensitive lookups
for member in cls:
if member.value.lower() == value.lower():
return member
return NoneThen we could move the documentation for each into the MatchScope docstr and say refer to :class:MatchScope to make each method docstr more concise.
There was a problem hiding this comment.
I propose in that case that we change the name to scoping_policy.
As of now end-users (assuming that they only ever use kernprof.py) indeed has no way of changing it since they won't be directly calling LineProfiler.add_class(), LineProfiler.add_module(), or line_profiler.autoprofile.line_profiler_utils.add_imported_function_or_module(), but that can (and maybe should) change with #335. But yes maybe at this moment YAGNI.
StrEnum sounds like a good idea, will do.
There was a problem hiding this comment.
I'm ok with scoping_policy. I'm not in love with it, but I can't think of anything better. It might be worth sleeping on it and trying to come up with a more intuitive name. I feel like this arg is going to cause confusion, but I understand why its useful and I think we should have it.
It makes sense that pyproject toml will expose the option to the user.
There was a problem hiding this comment.
Once we've set up the utils module I guess it will also make sense to update #335 so that line_profiler.cli_utils is merged into that, but that's something for future us to worry about.
tests/test_eager_preimports.py
Outdated
| @@ -0,0 +1,190 @@ | |||
| """ | |||
| Tests for `line_profiler.autoprofile.eager_preimports`. | |||
There was a problem hiding this comment.
Wait, are we not running doctests in the CI?
That's a huge oversight on my part if that is the case - or I'm forgetting a reason for not having it. The test requirements contain xdoctest, and I also see --xdoctest in .github/workflows/tests.yml, but it's not in run_tests.py, which it probably should be.
I don't think we should need to add this file. Not sure why it's called test_eager_preimports.py am I missing something?
There was a problem hiding this comment.
Ah yes I missed that we do have doctests in CI via xdoctest. In that case most of this test module is probably unnecessary. However:
- The module is for testing
line_profiler/autoprofile/eager_preimports.py, hence the name. - There's currently one bona-fide test (
test_write_eager_import_module_wrong_adder()) here which isn't a doctest wrapper. If we don't keep the module we might want to think about whither to rehabilitate it.
There was a problem hiding this comment.
Ah, I missed that small test at the end. Let's keep test_write_eager_import_module_wrong_adder in this file, and remove everything else.
I should probably update pyproject.toml to add xdoctest by default. I'll do that in a different PR.
tests/test_explicit_profile.py
Outdated
| @@ -7,6 +8,15 @@ | |||
| import ubelt as ub | |||
|
|
|||
|
|
|||
| @contextlib.contextmanager | |||
| def enter_tmpdir(): | |||
There was a problem hiding this comment.
Let's make this class-based for my own sanity. I find it too easy to get confused with contextlib decorators.
There was a problem hiding this comment.
Ah yes, considering the discussions we've had over #340. Will do.
tests/test_autoprofile.py
Outdated
| (False, None, False, True, | ||
| False, False, False, False, False, False), | ||
| (True, None, False, True, | ||
| False, False, False, False, False, False)]) |
There was a problem hiding this comment.
Similar refeactor here:
@pytest.mark.parametrize(
['use_kernprof_exec', 'prof_mod', 'extra_args', 'expected_funcs'],
[
(False, 'test_mod.submod2,test_mod.subpkg.submod3.add_three',
['--no-preimports'],
['add_two']),
(False, 'test_mod.submod2,test_mod.subpkg.submod3.add_three',
[],
['add_two', 'add_three', 'add_operator']),
(False, 'test_mod.submod1',
[],
['add_one', 'add_operator']),
(False, 'test_mod.subpkg.submod4',
['--prof-imports'],
['add_one', 'add_two', 'add_four', 'add_operator', '_main']),
(False, None,
['--prof-imports'],
[]),
(True, None,
['--prof-imports'],
[]),
]
)
def test_autoprofile_exec_module(use_kernprof_exec, prof_mod, extra_args, expected_funcs):
"""
Test the execution of a module.
"""
temp_dpath = ub.Path(tempfile.mkdtemp())
_write_demo_module(temp_dpath)
if use_kernprof_exec:
args = ['kernprof']
else:
args = [sys.executable, '-m', 'kernprof']
if prof_mod is not None:
args.extend(['-p', prof_mod])
args.extend(extra_args)
args.extend(['-l', '-m', 'test_mod.subpkg.submod4', '1', '2', '3'])
proc = ub.cmd(args, cwd=temp_dpath, verbose=2)
print(proc.stdout)
print(proc.stderr)
proc.check_returncode()
prof = temp_dpath / 'test_mod.subpkg.submod4.lprof'
args = [sys.executable, '-m', 'line_profiler', os.fspath(prof)]
proc = ub.cmd(args, cwd=temp_dpath)
raw_output = proc.stdout
print(raw_output)
proc.check_returncode()
all_possible_funcs = ['add_one', 'add_two', 'add_three', 'add_four', 'add_operator', '_main']
for func in all_possible_funcs:
assert (f'Function: {func}' in raw_output) == (func in expected_funcs)There was a problem hiding this comment.
Great idea, yeah all the positional args in the tests are getting a bit out of hand. Will do.
tests/test_autoprofile.py
Outdated
| (False, None, False, True, | ||
| False, False, False, False), | ||
| (True, None, False, True, | ||
| False, False, False, False)]) |
There was a problem hiding this comment.
This is why I don't like pytest parameterize. It gets so messy. For some reason I'm having a hard time with the suggestion github feature, but I'm thinking a refactor like this might make it slightly easier to read:
@pytest.mark.parametrize(
['use_kernprof_exec', 'prof_mod', 'extra_args', 'expected_funcs'],
[
(False, 'test_mod.submod1', [], ['add_one', 'add_operator']),
# By using --no-preimports, only explicitly listed prof_mod is profiled
(False, 'test_mod.submod1', ['--no-preimports'], ['add_one']),
(False, 'test_mod.submod2', ['--prof-imports'], ['add_two', 'add_operator']),
(False, 'test_mod', ['--prof-imports'], ['add_one', 'add_two', 'add_operator', '_main']),
# Multiple -p modules without --prof-imports
(False, ['test_mod', 'test_mod.submod1,test_mod.submod2'], [], ['add_one', 'add_two', 'add_operator', '_main']),
(False, None, ['--prof-imports'], []),
(True, None, ['--prof-imports'], []),
]
)
def test_autoprofile_exec_package(use_kernprof_exec, prof_mod, extra_args, expected_funcs):
"""
Test the execution of a package.
"""
temp_dpath = ub.Path(tempfile.mkdtemp())
_write_demo_module(temp_dpath)
if use_kernprof_exec:
args = ['kernprof']
else:
args = [sys.executable, '-m', 'kernprof']
if prof_mod is not None:
if isinstance(prof_mod, str):
prof_mod = [prof_mod]
for pm in prof_mod:
args.extend(['-p', pm])
args.extend(extra_args)
args.extend(['-l', '-m', 'test_mod', '1', '2', '3'])
proc = ub.cmd(args, cwd=temp_dpath, verbose=2)
print(proc.stdout)
print(proc.stderr)
proc.check_returncode()
prof = temp_dpath / 'test_mod.lprof'
args = [sys.executable, '-m', 'line_profiler', os.fspath(prof)]
proc = ub.cmd(args, cwd=temp_dpath)
raw_output = proc.stdout
print(raw_output)
proc.check_returncode()
all_possible_funcs = ['add_one', 'add_two', 'add_operator', '_main']
for func in all_possible_funcs:
assert (f'Function: {func}' in raw_output) == (func in expected_funcs)Double check that I didn't miss something in this refactor. I used ChatGPT to generate it.
line_profiler/line_profiler.py
Outdated
| return count | ||
|
|
||
| @staticmethod | ||
| def _add_module_filter(mod, match_scope): |
There was a problem hiding this comment.
Feels weird to have these as staticmethods. Maybe if we change the match_scope to a StrEnum, we can add these as methods there?
line_profiler/line_profiler.py
Outdated
| @@ -28,9 +31,28 @@ | |||
| # NOTE: This needs to be in sync with ../kernprof.py and __init__.py | |||
| __version__ = '4.3.0' | |||
|
|
|||
| # These objects are callables, but are defined in C so we can't handle | |||
| # them anyway | |||
| c_level_callable_types = (types.BuiltinFunctionType, | |||
There was a problem hiding this comment.
Data constant should generally be all caps.
There was a problem hiding this comment.
I've heard opposing views about this, but yes personally I agree and I do the same in my personal projects. Since you gave your blessing, will do it here.
| if not options.prof_mod: | ||
| options.no_preimports = True | ||
| if options.line_by_line and not options.no_preimports: | ||
| # We assume most items in `.prof_mod` to be import-able without |
There was a problem hiding this comment.
Let's factor this new block out into its own function to try to minimize how long each individual function is. This could probably be moved into the autoprofile.eager_preimports submodule.
There was a problem hiding this comment.
... yeah this is obvious in hindsight, that function was getting a bit too big. Will do.
|
Sorry for taking this long, I kinda got distracted by trying to get One thing that I've noticed is that EDIT: while :py:deco:`profile`is correctly rendered into .. E.g. this was in ``docs/source/auto/line_profiler.rst``
.. py:decorator:: line_profiler.profile
:py:class:`~.GlobalProfiler` instance.(I've since rolled the changes in the RST files back because the links don't work anyway.) And it wasn't an issue of duplicate references anyway, since even if I were to do :py:deco:`~.profile`and only keep one of the two |
|
Fun, I found that: does work to produce Will allow you to use custom text |
|
So I'm testing this out in a real world use-case. And its definitely working in that if I include enough Is that something that changing the scope policy could help with? Here is the example I was playing with: https://gist.github.com/Erotemic/3abd223c55b761b39fc7746ea4307faf I want to see where the webdataset and wids package are taking time. I didn't write them, but I want to use line-profiler to explore the code that's called based on my example. Another improvement I see that might help is that when we write the output for a specific method, we give the method name and the file it was from, but we don't report the name of the class that it belongs to. This is separate from this PR, but we should record and report this information. (I think we can get this behavior if we use |
Currently there are several idiosyncrasies with the
One fix could be that we provide separate scoping-policy switches for classes, modules, and functions. The current way
Recursive descension into submodules can also be finicky in that
To address the first, we can maybe use And for the second we may want to include a separate flag in
That should be all advantages to the end-user and I'm very much on board with it (in another PR as you've suggested). It will probably break some tests here in |
|
Addendum: just checked and |
|
I'm still working on ironing out the stuff we've discussed in the last three comments; will get back to you in a couple hours. |
|
I'm going to take a shot at writing a patch that will eager import all submodules when the argument to EDIT: Well, I may not get to the actual patch to this lib, but I can point to the code that can make it happen. It was in xdoctest, not mkinit, and it was already ported to this package in |
|
Sounds interesting, I'll take a look at that, thanks for the pointers. Any input on the :py:class:`StrEnum` for scoping policies, that is, how it is
decided whether to:
* Profile a function found in a namespace (a class or a module), and
* Descend into nested namespaces so that their methods and functions
are profiled,
when using :py:meth:`LineProfiler.add_class`,
:py:meth:`LineProfiler.add_module`, and
:py:func:`~.add_imported_function_or_module()`.
Available policies are:
:py:attr:`ScopingPolicy.EXACT`
Only profile *functions* found in the namespace fulfilling
:py:attr:`ScopingPolicy.CHILDREN` as defined below, without
descending into nested namespaces
:py:attr:`ScopingPolicy.CHILDREN`
Only profile/descend into *child* objects, which are:
* Classes and functions defined *locally* in the very
module, or in the very class as its "inner classes" and
methods
* Direct submodules, in case when the namespace is a module
object representing a package
:py:attr:`ScopingPolicy.DESCENDANTS`
Only profile/descend into *descendant* objects, which are:
* Child classes, functions, and modules, as defined above in
:py:attr:`ScopingPolicy.CHILDREN`
* Their child classes, functions, and modules, ...
* ... and so on
:py:attr:`ScopingPolicy.SIBLINGS`
Only profile/descend into *sibling* and descendant objects,
which are:
* Descendant classes, functions, and modules, as defined above
in :py:attr:`ScopingPolicy.DESCENDANTS`
* Classes and functions (and descendants thereof) defined in the
same parent namespace to this very class, or in modules (and
subpackages and their descendants) sharing a parent package
to this very module
* Modules (and subpackages and their descendants) sharing a
parent package, when the namespace is a module
:py:attr:`ScopingPolicy.NONE`
Don't check scopes; profile all functions found in the local
namespace of the class/module, and descend into all nested
namespaces recursively
Note:
This is probably a very bad idea for module scoping;
proceed with care.so the previous |
|
I don't think you need siblings. You could just target the common parent. You always have a tree right? So their can't be more than one parent? Examples or a table like what is shown: https://chatgpt.com/share/6831118e-2f34-8002-b371-6233b0c5132c would be helpful. |
|
The point for siblings was, e.g.
By setting the scoping policy to
Hence the user don't need to know that where |
line_profiler/autoprofile/eager_preimport.py[i]
New module for generating a dummy module where the profiling targets
are pre-imported, so that they can be added to the profiler;
main functionalities:
- `split_dotted_path()`: split a dotted path into a module part and
an attribute part
- `write_eager_import_module()`: write the text of a module which
does all the supplied imports and adds them to the profiler
tests/test_eager_preimports.py
create_doctest_wrapper(), regularize_doctests()
New functions to create hooks running doctests, even when
`--doctest-modules` or `--xdoctest` is not passed
test_doctest_*()
Hook tests for the `line_profiler.autoprofile.eager_preimports`
doctest
test_write_eager_import_module_wrong_adder()
Test for passing bad `adder` values to
`write_eager_import_module()`
kernprof.py
__doc__
Updated with the new option
main()
Added new option `-e`/`--eager-preimports` for eagerly importing
the `--prof-mod` targets, so that they are all unconditionally
profiled (where possible) regardless of whether they are
imported in the test script/module
|
This can be a big PR. We want to make sure we get things right. It will be a major improvement in the software. |
kernprof.py
main()
--verbose
New flag (`-v` and `--view` now aliases thereto) for
increasing verbosity (old `--view` behavior equivalent to
level 1; diagnostic output at level 2)
-q, --quiet
New flag for decreasing verbosity (e.g. suppressing help
messages or output of the profiled script)
_write_tempfile()
Now printing the location of the tempfile as diagnostic output
_write_preimports()
- Changed call signature
- Now printing the generated file as diagnostic output
- Now using `rich` (where available and requested) to highlight
the generated file
kernprof.py::main()
--verbose
- Now prefixing diagnostic output with '[kernprof <timestamp>]'
- Added diagnostic outputs for:
- Implicitly chosen `--outfile` destination
- Function call used to run the profiled code
--rich
- Now unsetting the flag when `rich` cannot be imported
- Now using `rich` to highlight all diagnostics if set
TODO: tests
kernprof.py
main()
- Made the formatting of code more consistent between `--rich`
and non-rich mode
- Added more diagnostic output regarding the script run and the
parsed arguments
_write_tempfile()
Added (syntax-highlighted) diagnostic output for the tempfile
_main()
- Refactored internal function used for formatting function
calls, using `pprint.pformat()` to indent the function-call
arguments
- Replaced use of `locals()` with explicitly-constructed
namespaces
|
Went with making Will add tests later. |
kernprof.py::__doc__
Updated `kernprof --help` output
tests/test_kernprof.py::test_kernprof_verbosity()
New test for verbosity and output
line_profiler/autoprofile/eager_preimports.py
write_eager_import_module()
- Added "# noqa: F821" to the module line defining `adder`
- Added one extra blank line between preambles and imports
(PEP8)
- Fixed bug where errors importing a module are not shown in the
warning listing the failed profiling targets
- Now distinguishing between explictly provided and implicitly
included (via `recurse`) profiling targets, allowing arbitrary
errors (instead of just `ImportError`) when importing the
latter
tests/test_eager_preimports.py
test_write_eager_import_module_wrong_adder()
- Removed unused parameter `msg`
- Added type annotations for the parameters
test_written_module_pep8_compliance()
New test that the generated module passes linting
test_written_module_error_handling()
New test that failures in retrieving the profiling targets are
appropriately converted into errors and warnings
CHANGELOG.rst
Updated entry
tests/test_kernprof.py
Loosened problematic subtest which trigger a `CoverageWarning` in CI
and fails (because it didn't expect any `stderr` output)
|
Updates:
Your edge case has been fixed: (3.13-eager-prof-mod) $ kernprof -l -pubelt -c "import ubelt"
<...>/kernprof.py:172: UserWarning: 1 target cannot be imported: ['ubelt._win32_jaraco']
exec(compile(f.read()), filename, 'exec', globals, locals)
Wrote profile results to '<...>/kernprof-command-<...>.lprof'
Inspect results with:
python -m line_profiler -rmt <...>/kernprof-command-<...>.lprof |
line_profiler/profiler_mixin.py[i]
C_LEVEL_CALLABLE_TYPES
Added type for Cython callables
is_c_level_callable()
Now also returns true for Cython callables
tests/test_line_profiler.py::test_add_class_wrapper()
Added test case for Cython callables
kernprof.py
Outdated
| # so we write to a tempfile and `execfile()` it | ||
| # - While this works theoretically for preserving traceback, the | ||
| # catch is that the tempfile will already have been deleted by the | ||
| # time the traceback is formatted; |
There was a problem hiding this comment.
What if we didn't use the TemporaryDirectory context manager, and instead carefully cleaned up after kernprof finished? Perhaps we could use an atexit callback to still keep it reasonably simple?
Don't make this change yet, I'm just wondering if that would be enough to avoid manually printing the error? Does this impact viewing the lprof file at all? Because at that point if kernprof cleaned up at all the tempdir would be gone.
I'm also considering adding some developer-focused environment variables to control diagnostic output similar to how I'm doing it in xdoctest.global_state.
There was a problem hiding this comment.
I'm currently writing something like this to review the code.
There was a problem hiding this comment.
Will have to think over this a bit more carefully.
Apropos, there's something that I've been thinking about. Given that we regularly have several simultaneous PRs which are gradually merged, recently I've been rebasing on main and force-pushing whenever a new one is merged. However, I realized that it may make gradual reviews more difficult since it probably resets whatever marker you have on your end regarding which changes have been looked at. Would it make your life easier if we only rebased after the PR is in principle approved?
There was a problem hiding this comment.
At this point yes. Sometimes it does help to rebase if a new PR makes testing easier, but yes let's wait to rebase.
There was a problem hiding this comment.
Re: your earlier comments:
- I'm playing around with some tests, and yes switching to an
atexit.register()hook does allow for the file to persist until the traceback has been formatted. However, this does have the drawback in that callingkernprof.main()multiple times in the same interpreter will cause a buildup of tempdirs. But I suppose that's a corner case and still acceptable as long as cleanup happens somewhere down the road (which it does). - The tempdir used in
_write_preimports()(containing the pre-import module) is separate from the the tempdir whitherkernprof -candkernprof -writes to (containing the executed script). By the time when the profiling results are shown, the former would be long gone. This doesn't affect profile-file viewing because the pre-import tempfile isn't supposed to be profiled at all to begin with. - What however does affect result viewing is when the user do
kernprof -corkernprof -without-v. With-vweprint_stats()before cleanup, so the executed script persists and can be shown. However, afterkernprof.main()exits the script is gone, and later visualization withpython -m line_profilerwill fail to find the profiled lines, rendering them as warnings (Could not find file ...) – which is IMO intended since the executed script is supposed to be transient anyway. One optimization to be made may be to only subject the temp script to whole-file profiling if we're viewing the results, so that users only interested in profiling other components with the script as a proxy won't get the warning message when they laterpython -m line_profiler. But of course this can also be a separate PR.
EDIT: if that is what you asked about, the problem with the non-preservation of tempfiles at traceback-formatting time (due to the use of tempfile.TemporaryDirectory) is shared by kernprof -c and kernprof -:
(3.13-eager-prof-mod) $ kernprof -c "(None for _ in []).throw(Exception('foo'))"
Wrote profile results to '/Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof-command-ldgun6j1.prof'
Inspect results with:
python -m pstats /Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof-command-ldgun6j1.prof
Traceback (most recent call last):
File "/Users/terence/python_env/line_profiler_dev/3.13-eager-prof-mod/bin/kernprof", line 8, in <module>
sys.exit(main())
~~~~^^
File "/Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof.py", line 357, in wrapper
return func(*args, **kwargs)
File "/Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof.py", line 357, in wrapper
return func(*args, **kwargs)
File "/Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof.py", line 644, in main
_main(options, module)
~~~~~^^^^^^^^^^^^^^^^^
File "/Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof.py", line 915, in _main
call_with_diagnostics(
~~~~~~~~~~~~~~~~~~~~~^
prof.runctx, f'execfile_({script_file!r}, globals())',
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ns, ns)
^^^^^^^
File "/Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof.py", line 786, in call_with_diagnostics
return func(*args, **kwargs)
File "/Users/terence/python_env/line_profiler_dev/eager-prof-mod/line_profiler/profiler_mixin.py", line 461, in runctx
exec(cmd, globals, locals)
~~~~^^^^^^^^^^^^^^^^^^^^^^
File "<string>", line 1, in <module>
File "/Users/terence/python_env/line_profiler_dev/eager-prof-mod/kernprof.py", line 173, in execfile
exec(compile(f.read(), filename, 'exec'), globals, locals)
~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/var/folders/5b/44kj9cdn7gngz2k1z1wxt0240000gp/T/tmp4kqf1d0d/kernprof-command.py", line 1, in <module>
File "/var/folders/5b/44kj9cdn7gngz2k1z1wxt0240000gp/T/tmp4kqf1d0d/kernprof-command.py", line 1, in <genexpr>
Exception: fooWill also fix that.
EDIT 12 Jun (Just putting a pothole here so I can hop between the this PR and yours quicker: TTsangSC#1)
kernprof.py::_write_preimports()
- Replaced `tempfile.TemporaryDirectory` context with explicit
`try: ... except ...: ...` blocks so that:
- If the temporary pre-import module is written but its execution
failed, its deletion is deferred to an `atexit` hook so that the
tempfile is preserved at traceback-formatting time
- Otherwise, the tempfiles are scrubbed as soon as possible,
e.g. when the pre-imports cannot be written or when they are
successfully executed
- Removed manual formatting of tracebacks
tests/test_kernprof.py::test_kernprof_eager_preimport_bad_module()
New test ensuring that the pre-import tempfile exists at
traceback-formatting time
kernprof.py
main()
- Moved tempfile managerment code here
- Added note in docstring
_touch_tempfile()
New convenience function wrapping around `tempfile.mkstemp()`
_write_tempfile()
Updated function signature and implementation
_write_preimports()
Simplified implementation (because the cleanup code is moved
outside to `main()`
tests/test_kernprof.py
test_kernprof_eager_preimport_bad_module()
Updated docstring
test_kernprof_bad_temp_script()
New test analogous to the above checking that the temp script is
available at traceback-formatting time
kernprof.py::_main()
Instead of directly calling `prof.dump_stats()`, now filtering the
stats so as not to include profiling data from tempfiles created in
`kernprof.main()`;
this removes the "Could not find file ..." error messages when the
profile data is later read with `python -m line_profiler`
tests/test_autoprofile.py::test_autoprofile_from_stdin()
Updated to include check for the above
[wip] Debugging
Eager prof mod tempfile updates
line_profiler/_diagnostics.py
- Removed unused comments
- Added default name (`'line_profiler'`) to `.log`
line_profiler/_logger.py
_LogBackend
- Added class attribute `.backend`
- Added abstract method `.configure()`
_PrintLogBackend.configure()
Now optionally taking `level` as the first argument like how
`_StdlibLogBackend.configure()` does
_StdlibLogBackend.configure()
Now taking (and ignoring) arbitrary keyword arguments as
specfied in `_LogBackend.configure()`
Logger.configure()
Updated and simplified implementation
kernprof.py
_restore
- Refactored from `_restore_list`
- New class methods `.sequence()`, `.mapping()`, and
`.instance_dict()` for restoring various objects
_remove()
New utility function for deleting files and directories
main()
"Developer mode" options (note: all of the below should be
considered implementational details)
- Refactored internal methods for printing messages and
diagnostics to use `line_profiler._diagnostics.log`
- Now outputting diagnostics regardless of verbosity level if
`line_profiler._diagnostics.DEBUG`
- Now keeping the temporary files if
`line_profiler._diagnostics.KEEP_TEMPDIRS`
- Now not executing any source file (setup, pre-imports,
profiled code) nor writing profile output if
`line_profiler._diagnostics.NO_EXEC`
line_profiler/autoprofile/eager_preimports.py[i]
__all__
New module attribute
split_dotted_path()
Added new argument `static` for choosing whether to use static
analysis
(`line_profiler.autoprofile.util_static.modname_to_modpath()`)
or the import system (`importlib.util.find_spec()`) to resolve
dotted paths
resolve_profiling_targets()
Split from `write_eager_import_module()` for easier testing
write_eager_import_module()
- Fixed malformed RST in docstring
- Added new argument `static` for choosing whether to use static
analysis or the import system to:
- Resolve dotted paths (see `split_dotted_path()`), and
- Find subpackages and -modules
(`~.util_static.package_modpaths()` vs
`pkgutil.walk_packages()`)
tests/test_eager_preimports.py
sample_package()
- Renamed from `sample_module()`
- Simplified implementation
preserve_sys_state(), sample_namespace_package()
New fixtures
gen_names()
New utility function
test_{split_dotted_path,resolve_profiling_targets}_staticity()
New tests for how the `static` parameter influences the behavior
of
`line_profiler.autoprofile.eager_preimports.split_dotted_path()`
and `.resolve_profiling_targets()`
kernprof.py::main(), _write_preimports()
Now choosing whether to use static-only analysis when writing the
pre-import module by `line_profiler._diagnostics.STATIC_ANALYSIS`
line_profiler/_diagnostics.py::STATIC_ANALYSIS
New "dev-mode" variable set by the environment variable
`${LINE_PROFILER_STATIC_ANALYSIS}`
line_profiler/line_profiler.py[i]
LineProfiler.add_callable()
- New argument `name` for referring to the added object in log
messages
- Now emitting a log message for each of the underlying
functions
LineProfiler.add_class(), .add_module()
Now emitting a log message if any member in the class/module
has been added
line_profiler/profiler_mixin.py[i]
is_cython_callable()
- Now a separate function
- Now checking the name of the object's type instead of doing an
instance check, so that we retain compatibility with extension
modules built with different Cython versions
is_c_level_callable()
Now calls `is_cython_callable()`, instead of hard-coding
`type(line_profiler._line_profiler.label)` into
`C_LEVEL_CALLABLE_TYPES`
|
Just got everything merged:
The
So I just ended up adding the Another thing: should we be documenting the envvar switches, or do we not bother since they aren't meant for end-users (and can end up on the chopping board without notice) anyway? |
Synopsis
This PR aims to make the selection of auto-profiling targets more intuitive (see e.g. issue #318) by:
-eflag is removed) Making-p/--prof-mod"eager" by default, which essentially generates and loads an extra setup module to ensure that the supplied targets are all added to the profiler, regardless of whether they are directly imported by the run script/module or not.--no-preimportsflag.LineProfiler.add_module()and.add_imported_function_or_module()more aggressive in adding namespace members, instead of just giving up if the member isn't a class or atypes.FunctionType; this allows us to leverage PR FIX: (+ENH?) fixed and extended dispatching forLineProfiler.__call__()#332 to profile e.g. class methods and properties in imported modules/classes.LineProfiler(and by extension,GlobalProfiler) can now also be directly used as a class decorator.kernprofnow takes the new flags-v/--verbose(to which--viewis now aliased) and-q/--quiet, which allows for either outputting diagnostics or suppressing outputs.Code changes
kernprof.py:.__doc__find_script():Added optional argument
exit_on_errorso that we don't always have totry: ... except SystemExit: ...when using itmain():Updated help text for option
-p/--prof-mod(Added 18 May) Made
-ptargets eagerly pre-imported for profiling by default(Added 18 May) Added flag
--no-preimportsfor restoring the old behavior (only profile-ptargets that are imported in the executed script/module)(Added 18 May) Now clearing the
.enable_countand.disable()-ing the created profiler to further reduce the side effectsmain()has(Added 26 May) Eager pre-imports now recurse/descend into packages by default; this can be suppressed by specifying the import target as
<pkg>.__init__(Added 29 May) Replaced the
-v/--viewboolean flag with the following:-v/--verbose/--view: increments verbosity-q/--quiet: decrements verbosityThe verbosity levels are as follows:
2: show diagnostic output (e.g. the pre-import module written, the function call used for running the script)1: show profiling results (equivalent to the previous--view)0: default behavior (equivalent to the previous default)-1: suppresskernprofhelp messages (e.g.Wrote profile results to <filename>)-2: suppressstdoutof the executed code-3: suppressstderrof the executed code(Added 15 Jun) Tempfile removal (pre-imports, executed script) now deferred so that tracebacks are properly formatted
(Added 15 Jun) Profiling data from functions living in tempfiles are filtered out of the written
lproffile, so that users don't see theCould not find fileerror message when viewing the file withpython -m line_profiler(Added 15 Jun) The following "dev-mode" environmental switches now control the behavior of
main():${LINE_PROFILER_DEBUG}: ensures debugging/diagnostic output even at reduced verbosity levels${LINE_PROFILER_NO_EXEC}: does a "dry-run" without actually writing profiling results or executing the following:-s/--setup)-p/--prof-mod)${LINE_PROFILER_KEEP_TEMPDIRS}: keeps the tempfiles written (the pre-import file and the profiled script (if supplied via thestdinor the-cflag))${LINE_PROFILER_STATIC_ANALYSIS}: use only static and path-based analysis (line_profiler.autoprofile.util_static) to handle pre-imports, instead of going through the import system (importlib.utilandpkgutil)(Updated 19 May) Updated various docstrings to be more
sphinx-friendlyline_profiler/__init__.py:(Added 19 May) Updated various docstrings to be more
sphinx-friendlyline_profiler/_logger.py:(Merged 14 Jun; contributed by @Erotemic) New module for logging facilities
line_profiler/_diagnostics.py:(Merged 14 Jun; contributed by @Erotemic) New module for "dev-mode" switches, package-wide logging, etc.
line_profiler/autoprofile/eager_preimports.py[i]:New module for implementing eager pre-imports: explicitly importing all the profiling targets in a generated script to add them to the profiler
split_dotted_path():Function for determining where the module stops and the chained attribute access starts in a dotted path like
package.submodule.SomeClass.some_attributewrite_eager_import_module():Function for writing the module which imports the targets and adds them to the profiler
resolve_profiling_targets():(Added 15 Jun) Function for resolving dotted paths into the exact targets to import and pass to the profiler
line_profiler/autoprofile/line_profiler_utils.py[i]::add_imported_function_or_module():scoping_policyfor limiting what functions/-wrappers, classes, and modules when they are found as members to other classes and/or moduleswrapfor controlling whether to replace added class and module members with@LineProfiler.wrap_callablewrappersint(1 if anything has been added to the profiler, 0 otherwise) for consistency with the.add_callable(),.add_module(), and.add_class()methodsline_profiler/line_profiler_utils.py[i]:(Added 19 May) New module for utilities
StringEnum:Convenience subclass/backport of
enum.StringEnumline_profiler/line_profiler.py[i]:LineProfiler:.wrap_callable():(Updated 1 Jun) Now a no-op on C(-ython)-level callables (e.g. the various
typescallable types that aren'tFunctionTypeorMethodType).add_callable():typescallable types that aren'tFunctionTypeorMethodType)guardfor controlling what callables to pass onto.add_function()namefor logging purposes.add_function().add_module():match_scope) Added optional argumentscoping_policyfor limiting what functions/-wrappers, classes, and modules to descend into when they are found as members to other classes and/or moduleswrapfor controlling whether to replace added class and module members with@LineProfiler.wrap_callablewrappers.add_callable().add_class():New method (shares implementation with
.add_module())(Updated 19 May) Updated various docstrings to be more
sphinx-friendlyline_profiler/profiler_mixin.py[i]::ByCountProfilerMixin:sphinx-friendlywrap_callable():(Updated 27 May) Added handling for (1) classes and (2) callable wrappers (e.g.
classmethod) which wraps around classes instead of functions; by extension,LineProfilerandGlobalProfilercan now be used as class decoratorswrap_class():(Added 27 May) New method for wrapping around classes (by wrapping all its locally-defined methods and similar)
get_underlying_functions():(Added 27 May) New class method migrated and refactored from
line_profiler/line_profiler.py::_get_underlying_functions().__call__()method is passedline_profiler/scoping_policy.py[i]::ScopingPolicy:(Added 19 May; migrated from
line_profiler/line_profiler.py27 May) New string enum for documenting and implementing the valid values of thescoping_policyparameterDoc changes
docs/source/auto/line_profiler.rst,line_profiler.autoprofile.rst:(Updated 27 May) Added new doc pages to the indices
docs/source/auto/line_profiler.autoprofile.ast_profile_transformer.rst:(Updated 26 May) Renamed from the typo-ed
ast_profle_transformer.rst(see FIX: auto-profile transformer typo #325), fixing the disappearance of the page from the output HTMLdocs/source/auto/line_profiler.autoprofile.run_module.rst,autoprofile.eager_preimports.rst,profile_mixin.rst,scoping_policy.rst:(Updated 27 May) Added (missing) doc pages for preexisting and new modules
Test-suite changes
tests/test_autoprofile.py:test_autoprofile_exec_package(),test_autoprofile_exec_module():kernprof -p--no-preimportstest_autoprofile_callable_wrapper_objects():New test that the callable wrappers like class methods and properties are added to the profiler on import
tests/test_eager_preimports.py:New test module for
line_profiler/autoprofile/eager_preimports.pytest_write_eager_import_module_wrong_adder():Test that
write_eager_import_module()complains about badadder(callable to be used verbatim to add objects, i.e.'profile.add_imported_function_or_module') valuestest_written_module_pep8_compliance():(Added 29 May) Test that
write_eager_import_module()write a module that is PEP-8-compliant (requiresflake8)test_written_module_error_handling():(Added 29 May) Test the warning/error-raising behavior of
write_eager_import_module()and the written module:test_split_dotted_path_staticity(),test_resolve_profiling_targets_staticity():(Added 15 Jun) Test the different behaviors when handling dotted paths with pure static analysis and via the import system
test_doctest_*()tests/test_explicit_profile.py:test_profiler_add_methods():New test for the new
wrapargument ofLineProfiler.add_imported_function_or_module(),.add_module(), and.add_class()test_profiler_add_class_recursion_guard():New test for the handling of self-/mutually-referential classes
test_profiler_warn_unwrappable():(Added 27 Apr) New test for the warning issued when wrappers around added namespace members cannot be set back to the namespace
test_profiler_class_scope_matching():(Updated 24 May; supersedes the previous
test_profiler_scope_matching()) New test for how thescoping_policyargument limits descent into classestest_profiler_func_scope_matching()(resp.test_profiler_module_scope_matching()):(Added 24 May) Corresponding new tests for
scoping_policyand the profiling of functions (resp. descent into modules)tests/test_line_profiler.py:test_profiler_c_callable_no_op():(Updated 1 Jun) New test for the no-op on C(-ython)-level callables
test_class_decorator():(Added 27 May) New test for decorating classes
test_add_class_wrapper():(Added 27 May) New test for using
.add_callable()on a callable wrapper (classmethod) which wraps around a class instead of a normal functiontests/test_kernprof.py:test_kernprof_verbosity():(Added 29 May) New test for
kernprof's output at different verbosity levelsConflicts
This PR conflicts with #335 because both made (at times overlapping) modifications to
kernprof.pyand the test suite. (Should be easy to resolve though since I wrote both.)Acknowledgements
kernprof --prof-modfunctions.LineProfiler.add_imported_function_or_module()builds upon FIX: (+ENH?) fixed and extended dispatching forLineProfiler.__call__()#332 and is inspired by/back-ported frompytest_autoprofile.profiler.LineProfiler.