Skip to content

ENH: more intuitive profiling-target selection #337

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

TTsangSC
Copy link
Contributor

@TTsangSC TTsangSC commented Apr 21, 2025

Synopsis

This PR aims to make the selection of auto-profiling targets more intuitive (see e.g. issue #318) by:

  • (Updated 18 May; the previously-proposed -e flag 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.
  • (Added 18 May) The old behavior can be recovered by passing the --no-preimports flag.
  • Making methods like 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 a types.FunctionType; this allows us to leverage PR FIX: (+ENH?) fixed and extended dispatching for LineProfiler.__call__() #332 to profile e.g. class methods and properties in imported modules/classes.

Code changes

  • kernprof.py:
    • Updated .__doc__
    • find_script():
      Added optional argument exit_on_error so that we don't always have to try: ... except SystemExit: ... when using it
    • main():
      • Updated help text for option -p/--prof-mod
      • (Added 18 May) Made -p targets eagerly pre-imported for profiling by default
      • (Added 18 May) Added flag --no-preimports for restoring the old behavior (only profile -p targets that are imported in the executed script/module)
      • (Added 18 May) Now clearing the .enable_count and .disable()-ing the created profiler to make further reduce the side effects main() has
    • Misc:
      (Updated 19 May) Updated various docstrings to be more sphinx-friendly
  • line_profiler/__init__.py:
    (Added 19 May) Updated various docstrings to be more sphinx-friendly
  • 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_attribute
    • write_eager_import_module():
      Function for writing the module which imports the targets and adds them to the profiler
  • line_profiler/autoprofile/line_profiler_utils.py[i]::add_imported_function_or_module():
    • (Updated 19 May) Added optional argument scoping_policy for limiting what classes to descend into when they are found as members to other classes and/or modules
    • Added optional argument wrap for controlling whether to replace added class and module members with @LineProfiler.wrap_callable wrappers
    • Changed return type of to int (1 if anything has been added to the profiler, 0 otherwise) for consistency with the .add_callable(), .add_module(), and .add_class() methods
    • Refactored to permit the adding of callable wrappers (e.g. class methods and properties), nested classes, etc.
  • line_profiler/line_profiler_utils.py[i]:
    (Added 19 May) New module for utilities
    • StringEnum:
      Convenience subclass/backport of enum.StringEnum
  • line_profiler/line_profiler.py[i]:
    • ScopingPolicy:
      (Added 19 May) New string enum for documenting and implementing the valid values of the scoping_policy parameter
    • LineProfiler:
      • .wrap_callable():
        (Added 27 Apr) Now a no-op on C-level callables (e.g. the various types callable types that aren't FunctionType or MethodType)
      • .add_callable():
        • Updated return-type annotation
        • (Added 27 Apr) Now a no-op (and correctly returns 0) on C-level callables (e.g. the various types callable types that aren't FunctionType or MethodType)
      • .add_module():
        • (Updated 19 May; supersedes the previous match_scope) Added optional argument scoping_policy for limiting what classes to descend into when they are found as members to other classes and/or modules
        • Added optional argument wrap for controlling whether to replace added class and module members with @LineProfiler.wrap_callable wrappers
        • Refactored to permit the adding of callable wrappers (e.g. class methods and properties), nested classes, etc.
        • Added recursion/duplication check so that self- and mutually-referential namespaces don't cause problems
      • .add_class():
        New method (shares implementation with .add_module())
      • Misc:
        (Updated 19 May) Updated various docstrings to be more sphinx-friendly

Test-suite changes

  • tests/test_autoprofile.py:
    • test_autoprofile_exec_package(), test_autoprofile_exec_module():
      • (Updated 18 May) Updated in accordance with the new behavior of kernprof -p
      • (Updated 19 May) Refactored to simplify the parametrization signatures
      • (Added 18 May) Added subtests for --no-preimports
    • test_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.py
    • test_write_eager_import_module_wrong_adder():
      Test that write_eager_import_module() complains about bad adder (callable to be used verbatim to add objects, i.e. 'profile.add_imported_function_or_module') values
    • (Removed 19 May) test_doctest_*()
  • tests/test_explicit_profile.py:
  • tests/test_line_profiler.py:

Conflicts

This PR conflicts with #335 because both made (at times overlapping) modifications to kernprof.py and the test suite. (Should be easy to resolve though since I wrote both.)

Acknowledgements

Copy link

codecov bot commented Apr 21, 2025

Codecov Report

Attention: Patch coverage is 84.10042% with 38 lines in your changes missing coverage. Please review.

Project coverage is 69.37%. Comparing base (0ab4471) to head (0e56350).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
line_profiler/line_profiler.py 78.57% 16 Missing and 5 partials ⚠️
line_profiler/autoprofile/eager_preimports.py 92.03% 3 Missing and 6 partials ⚠️
line_profiler/line_profiler_utils.py 62.50% 6 Missing ⚠️
line_profiler/autoprofile/line_profiler_utils.py 83.33% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #337      +/-   ##
==========================================
+ Coverage   64.84%   69.37%   +4.53%     
==========================================
  Files          13       15       +2     
  Lines        1041     1257     +216     
  Branches      228      270      +42     
==========================================
+ Hits          675      872     +197     
- Misses        306      321      +15     
- Partials       60       64       +4     
Files with missing lines Coverage Δ
line_profiler/__init__.py 100.00% <ø> (ø)
line_profiler/autoprofile/line_profiler_utils.py 85.71% <83.33%> (+35.71%) ⬆️
line_profiler/line_profiler_utils.py 62.50% <62.50%> (ø)
line_profiler/autoprofile/eager_preimports.py 92.03% <92.03%> (ø)
line_profiler/line_profiler.py 77.39% <78.57%> (+5.41%) ⬆️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b34b801...0e56350. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Erotemic
Copy link
Member

Erotemic commented Apr 29, 2025

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 python -m kernprof -lvr -e calendar -m calendar would be able to demo using the stdlib calendar main, and it does somewhat but it the way we are handling decorators seems like it's not working with the global enum they use?

However, I tested: python -m kernprof -lvr -e uuid -m uuid, and that did seem to work nicely.

@TTsangSC
Copy link
Contributor Author

TTsangSC commented Apr 29, 2025

It seems that global enum is indeed problematic, but the problem lies deeper than kernprof:

>>> 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/[email protected]/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 813, in <module>
    main()
    ~~~~^^
  File "/opt/homebrew/Cellar/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/3.13.2/Frameworks/Python.framework/Versions/3.13/lib/python3.13/calendar.py", line 289, in monthdays2calendar
    days = list(self.itermonthdays2(year, month))
  File "/opt/homebrew/Cellar/[email protected]/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/[email protected]/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/[email protected]/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 defined

Since runpy is the standard way of doing the equivalent of python -m in code, I'd say that is problematic behavior either on the part of runpy or enum. Apparently someone got a similar error in the discussion of python/cpython#103935 but got brushed aside since it didn't exactly have to do with the issue at hand...

The problem seems to be that @enum.global_enum does some witchery to sys.modules to retrieve the module whose globals should be modified, instead of going through the stack frames to retrieve it (which of course has its own issues). Since runpy.run_module() doesn't touch sys.modules by default, the enums are inserted into the wrong namespace, hence the error.

Calling runpy.run_module() with alter_sys=True seems to solve the issue; will write a separate PR to fix that later today.

@Erotemic
Copy link
Member

Conflicts need to be fixed here.

I'm also not sure of adding both -p and -e. It seems like we might want to just make -p be eager by default and then maybe have a flag for the user to opt-out of preimporting (e.g. --preimport=False or --no-preimport).

@TTsangSC
Copy link
Contributor Author

Fair enough, since the eager behavior is the more intuitive one; will do.

@TTsangSC
Copy link
Contributor Author

TTsangSC commented May 18, 2025

Dunno why but the CI tests seem substantially more sluggish on newer Pythons:

Python version\Platform ubuntu macOS windows
3.8 18.95s 21.35s 31.40s
3.10 28.72s 34.74s 43.88s
3.12 31.08s 33.46s 50.26s
3.13 28.51s 32.30s 46.19s

It's even more egregious on my own machine:

Python version\Command run_tests.py (i.e. /coverage`) pytest (i.e. w/o coverage)
3.8 16.37s 8.62s
3.13 33.46s 23.51s

#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

  1. this brings us back to one of our old discussions – if that becomes problematic (unlikely as it is), we may have to consider migrating line_profiler._line_profiler._sys_monitoring_[de]register() to C(-ython) implementations .

@Erotemic
Copy link
Member

Hmm, I ran ./run_tests.py twice on main on my local venvs. I got:

  • 3.13: 17.30s, 17.26s
  • 3.12: 17.24s, 17.10s
  • 3.11: 11.97s, 11.86s
  • 3.10: 11.13s, 10.76s
  • 3.9: 11.36s, 11.06s

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... :)

@TTsangSC
Copy link
Contributor Author

TTsangSC commented May 18, 2025

Making line_profiler "self-hosting" – being able to profile itself – will be the holy grail. IDK how doable that will be or if it's even possible (maybe not, due to the highly probable recursion shenanigans), but it also feels somewhat in reach considering the progress we've been 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:

  • For in-process tests, that would imply having more than one profilers active. That is currently not allowed (in 3.12+) thanks to ENH: sys.monitoring compliance #327, but even if we were to be more lax with that and allow all LineProfiler instances to share the sys.monitoring lock:
  • For subprocess tests (the majority of the test suite, at least the part which calls kernprof), there's obviously no way for the profiler to track them.

Maybe I can try to do as coverage does to combine profiling data from different Python processes (see also #219, which IMO seems like a good idea but needs to be developed), but I haven't got that figured out yet, since we need to know when a Python process is started in order to set up the instrumentation. Or maybe I'm overthinking it and we can either:

  • Set up explicit profiling throughout line_profiler (or a copy thereof) and use LINE_PROFILE=1, or
  • Generate a good ol' shimmed-in .pth file on-the-fly so that the new process automatically sets up the profiling tooling.

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 line_profiler.line_profiler.LineProfiler just:

  • Be a wrapper over it – which it kinda already is – instead of inheriting from it, and either
  • Subscribe from each instance to the functions that it is profiling, or
  • Handle the bookkeeping and filtering of profiling data in each instance's get_stats().

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.

Copy link
Member

@Erotemic Erotemic left a comment

Choose a reason for hiding this comment

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

Finally got to this. I was able to review everything. Once we take care of these comments we can merge and move to the next one.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes a lot of sense. I agree with minimizing side effects and looking at kernprof as a module.

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):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

'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
Copy link
Member

Choose a reason for hiding this comment

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

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 None

Then we could move the documentation for each into the MatchScope docstr and say refer to :class:MatchScope to make each method docstr more concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@TTsangSC TTsangSC May 18, 2025

Choose a reason for hiding this comment

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

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.

@@ -0,0 +1,190 @@
"""
Tests for `line_profiler.autoprofile.eager_preimports`.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -7,6 +8,15 @@
import ubelt as ub


@contextlib.contextmanager
def enter_tmpdir():
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this class-based for my own sanity. I find it too easy to get confused with contextlib decorators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, considering the discussions we've had over #340. Will do.

(False, None, False, True,
False, False, False, False, False, False),
(True, None, False, True,
False, False, False, False, False, False)])
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@TTsangSC TTsangSC May 18, 2025

Choose a reason for hiding this comment

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

Great idea, yeah all the positional args in the tests are getting a bit out of hand. Will do.

(False, None, False, True,
False, False, False, False),
(True, None, False, True,
False, False, False, False)])
Copy link
Member

Choose a reason for hiding this comment

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

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.

return count

@staticmethod
def _add_module_filter(mod, match_scope):
Copy link
Member

Choose a reason for hiding this comment

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

Feels weird to have these as staticmethods. Maybe if we change the match_scope to a StrEnum, we can add these as methods there?

@@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Data constant should generally be all caps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... yeah this is obvious in hindsight, that function was getting a bit too big. Will do.

@TTsangSC
Copy link
Contributor Author

TTsangSC commented May 19, 2025

Sorry for taking this long, I kinda got distracted by trying to get sphinx to work.

One thing that I've noticed is that EDIT: while

:py:deco:`profile`

is correctly rendered into @profile, it fails to resolve to either @line_profiler.profile or @line_profiler.explicit_profiler.profile despite my adding the latter to the corresponding files in docs/source/auto/, like

.. 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 .. py:decorator:: ... blocks (so that there is no ambiguity which the ref should resolve to), it would get rendered into @~.profile and still doesn't get linked to the block. Maybe I should've just given up and used :py:data:...

@Erotemic
Copy link
Member

Erotemic commented May 19, 2025

Fun,

I found that:

:py:deco:`line_profiler.explicit_profiler.GlobalProfiler`

does work to produce @line_profiler.explicit_profiler.GlobalProfiler, and using ChatGPT I found that:

:py:deco:`profile <line_profiler.explicit_profiler.GlobalProfiler>`

Will allow you to use custom text @profile and overwrite the link location to GlobalProfiler.

TTsangSC added 14 commits May 20, 2025 22:54
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
kernprof.py
    __doc__
        Updated
    find_script()
        Added argument `exit_on_error` for when it should raise an error
        instead of quitting when a file is not found
    _normalize_profiling_targets()
        - Moved code for normalizing `--prof-mod` from `main()` to here
        - Added path resolution with `find_script()` and
          `os.path.abspath()`
    main()
        - Updated help texts for `-p`/`--prof-mod` and
          `-e`/`--eager-preimports`
        - Now permitting using `-e`/`--eager-preimports` with arguments,
          as with `-p`/`--prof-mod`;
          the no-arg form corrsponds to just taking the `--prof-mod`,
          while the with-arg form permits specifying different profiling
          targets between the two options
line_profiler/autoprofile/line_profiler_utils.py
    add_imported_function_or_module()
        - Added new argument `wrap` for controlling whether to replace
          class and module members with wrappers
        - Refactored object adding to be more aggressive, ditching the
          explicit `inspect.isfunction()` check (since we expanded the
          catalog of addable objects in pyutils#332)
        - Now returning whether any function has been added to the
          profiler

line_profiler/line_profiler.py
    add_module()
        - Now shares an implementation with `.add_class()`
        - Added new argument `wrap` for controlling whether to replace
          members with wrappers
        - Refactored object adding to be more aggressive, ditching the
          explicit `is_function()` check (since we expanded the catalog
          of addable objects in pyutils#332)
    add_class()
        New method (alias to `.add_module()`)
tests/test_autoprofile.py
    test_autoprofile_eager_preimports()
        New test for the behaviors of the `-e` and `-p` options
    test_autoprofile_callable_wrapper_objects()
        Test that on-import autoprofiling catches callable wrapper types
        like classmethods
tests/test_explicit_profile.py
    test_profiler_add_methods()
        New test for the `wrap` argument of the
        `add_imported_function_or_module()`, `.add_class()`, and
        `.add_module()` methods
line_profiler/line_profiler.py::LineProfiler.add_class(), .add_module()
    Added object-identity tracking to avoid possible recursion if the
    namespaces profiled reference themselves or one another

tests/test_explicit_profile.py
    test_profiler_add_class_recursion_guard()
        New test for adding self-/mutually-referential classes to be
        profiled
CHANGELOG.rst
    Added entry for the PR

tests/test_explicit_profile.py
    test_explicit_profile_with_duplicate_functions()
        Reworded docstring
tests/test_eager_preimports.py::create_doctest_wrapper()
    - Fixed compatibility error (`_pytest.doctest.get_optionflags()`
      expecting a `pytest.Config` in v8+ but an object having it at
      `.config` below)
    - Future-proofed against `pytest` API changes by falling back to the
      vanilla `doctest` implementation with a warning if setting up the
      test item fails
line_profiler/autoprofile/line_profiler_utils.py[i]
    add_imported_function_or_module()
        Added the `match_scope` argument for limiting the scope of
        descension into classes in namespaces (classes and modules)

line_profiler/line_profiler.py[i]
    is_c_level_callable()
        New check for non-profilable C-level callables
    LineProfiler
        add_callable(), wrap_callable()
            Now no-ops on C-level callables
        add_class(), add_module()
            - Added the `match_scope` argument for limiting the scope of
              descension into classes in namespaces (classes and
              modules)
            - Added handling for when the `setattr()` on the namespace
              fails
        __call__()
            Added missing method in stub file
line_profiler/line_profiler.py::LineProfiler.add_class(), add_module()
    Fixed implementation of internal method used (wrong interpretation
    of the `match_scope` parameter)

tests/test_explicit_profile.py
    test_profiler_add_methods()
    test_profiler_add_class_recursion_guard()
        Simplified implementations
    test_profiler_warn_unwrappable()
        New test for the warning in `LineProfiler.add_*(wrap=True)`
    test_profiler_scope_matching()
        New test for `LineProfiler.add_*(match_scope=...)`

tests/test_line_profiler.py::test_profiler_c_callable_no_op()
    New test for how the profiler leaves C-level callables untouched
TTsangSC added 13 commits May 20, 2025 22:54
CHANGERLOG.rst
    - Reworded previous entry (pyutils#338)
    - Reworded entry

kernprof.py
    __doc__
        Updated
    _normalize_profiling_targets.__doc__
    _restore_list.__doc__
    pre_parse_single_arg_directive.__doc__
        Reformatted to be more `sphinx`-friendly
    main()
        - Removed the `-e`/`--eager-preimports` flag
        - Made eager pre-imports the default for `--prof-mod`
        - Added new flag `--no-preimports` for restoring the old
          behavior
tests/test_autoprofile.py
    test_autoprofile_exec_package(), test_autoprofile_exec_module()
        Updated to reflect the new behavior of `kernprof` and to test
        the `--no-preimports` flag
    test_autoprofile_eager_preimports()
        Removed because the `--eager-preimports` flag is removed
    test_autoprofile_callable_wrapper_objects()
        Updated because the `--eager-preimports` flag is removed
line_profiler/line_profiler.py[i]::LineProfiler
    _add_namespace()
        - Signature changes:
          - Renamed: `match_scope` -> `scoping_policy`
          - Renamed: `duplicate_tracker` -> `seen`
          - `seen` now a keyword-only parameter
        - Internal refactoring to call the methods directly instead of
          pre-fetching them
    _add_{class,module}_filter(), add_{class,module}()
        Renamed parameter `match_scope` -> `scoping_policy`

line_profiler/autoprofile/line_profiler_utils.py[i]
    add_imported_function_or_module()
        Renamed parameter: `match_scope` -> `scoping_policy`

tests/test_explicit_profile.py::test_profiler_scope_matching()
    Updated because of the change in the signature of
    `LineProfiler.add_class()` and `.add_module()`
line_profiler/line_profiler_utils.py[i]
    New module for miscellaneous utilities used by `line_profiler`;
    currently containing `StringEnum`, a backport-slash-extension of
    `enum.StrEnum` in Python 3.11+
line_profiler/autoprofile/line_profiler_utils.py[i]
    add_imported_function_or_module()
        - Updated type annotation and default value for `scoping_policy`
        - Reformatted docstring to be more `sphinx`-friendly

line_profiler/line_profiler.py[i]
    C_LEVEL_CALLABLE_TYPES
        Renamed from `c_level_callable_types`
    is_c_level_callable()
        Updated return annotation to use `TypeIs` instead of `TypeGuard`
        since neither is in 3.8 anyway
    ScopingPolicy
        New string enum documenting and delineating valid values for
        `LineProfiler.add_*(scoping_policy=...)`
    LineProfiler.add_class(), .add_module()
        - Updated docstrings and type annotations
        - Now converting arugment `scoping_policy` to `ScopingPolicy`
          objects
    LineProfiler._add_class_filter(), ._add_module_filter()
        Moved implementations to `ScopingPolicy`
    LineProfiler.load_stats(), .dump_stats()
        Reformatted docstrings to be more `sphinx`-friendly
tests/test_eager_preimports.py
    create_doctest_wrapper(), regularize_doctests()
        Removed since the `line_profiler.autoprofile.eager_preimports`
        doctests are already run in CI, despite not being covered in
        `run_tests.py` by default
tests/test_explicit_profile.py::enter_tmpdir
    Now explicitly defined as a class, instead of using
    `@contextlib.contextmanager` (see pyutils#340)
tests/test_line_profiler.py::test_profiler_c_callable_no_op()
    - Updated docstring
    - Now a parametrized test testing the two no-ops (`.add_callable()`
      and `.__call__()`) separately
tests/test_autoprofile.py
    test_autoprofile_exec_{package,module,callable_wrapper_objects}()
        Refactored tests to simplify call/parametrization signatures
kernprof.py
    __doc__
        - Added `sphinx` roles to generic inlined code (e.g.
          ':command:', ':option:')
        - Rephrased documentation for the `-l` flag
    execfile.__doc__
    RepeatedTimer.__doc__
    find_script.__doc__
    _python_command.__doc__
    _normalize_profiling_targets.__doc__
    _restore_list.__doc__
    pre_parse_single_arg_directive.__doc__
    _write_tempfile.__doc__
    _main.__doc__
        Added `sphinx` roles to generic inlined code (e.g. ':command:',
        ':option:')
    _write_preimports()
        Refactored from big chunk of code in `_main()` handling the
        pre-imports
kernprof.py
    - Fixed broken link to `@line_profiler.profile` (or
      `line_profiler.explicit_profiler.GlobalProfiler`)
    - Suppressed link for the `-v` flag which linked to some nonsense

line_profiler/__init__.py
    - Added anchor to section `Line Profiler Basic Usage`
    - Replaced inlines with the appropriate roles
    - Fixed broken link to `@line_profiler.profile`
    - Fixed accidentally uncommented TODO
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants