Skip to content

Fable review of profile child process#5

Open
Erotemic wants to merge 9 commits into
TTsangSC:profile-child-processesfrom
Erotemic:fable-review-of-profile-child-process
Open

Fable review of profile child process#5
Erotemic wants to merge 9 commits into
TTsangSC:profile-child-processesfrom
Erotemic:fable-review-of-profile-child-process

Conversation

@Erotemic

@Erotemic Erotemic commented Jul 5, 2026

Copy link
Copy Markdown

@TTsangSC I had Fable perform an audit of the PR and the entire repo more generally. It wrote out a plan, but did the implementation for the trickiest parts itself.

There is also a new .llm_resource_tally which is a tool I'm working on to quantify and estimate the token and ultimately environmental costs of using agentic coding.

I still have to review this all, but I'm putting it on your radar. The main document relevant to your PR is: dev/planning/fable-review-pr431-2026-07-05.md and dev/planning/fable-plan-pr431-2026-07-05.md

Erotemic and others added 9 commits July 4, 2026 23:51
…filing)

Full review of the --prof-child-procs feature branch plus a broader
repo/CI/test-suite audit. Identifies three merge-blocking defects, each
reproduced locally:

- fork-based children re-dump inherited parent stats, double-counting
  all pre-fork hits/times in the merged output
- a silently-failed child .pth hook deadlocks patched pools via a
  result-protocol mismatch
- unwritable site-packages hard-crashes kernprof instead of degrading

Includes a prioritized remediation plan with per-task acceptance
criteria, test-suite and packaging/xcookie findings, and pre-existing
core bugs found along the way.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Vendors the self-contained tally tool, wires the shared post-commit
hook via core.hooksPath, and adds the managed AGENTS.md block. The
ledger is backfilled from the available session transcripts (the
review session that produced d1ecc3a and its subagent sessions);
lifetime-totals.json reflects the swept turns.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… docs

Splits the single PR pyutils#431 review into four dated documents:
- fable-review-pr431-2026-07-05.md: frozen PR-specific findings/evidence
- fable-review-fullrepo-2026-07-05.md: frozen repo-wide findings
- fable-pr431-plan-2026-07-05.md: live task board for the PR remediation
- fable-fullrepo-plan-2026-07-05.md: live task board for repo-wide work,
  including the new AST-pipeline robustness program and the .pyi-removal
  (modern inline annotations) program

The plan files are the coordination hub: agents claim tasks on the
status board, record outcomes in the agent log, and escalate decisions
via open questions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Forked children reuse the parent's profiler object (necessary: it holds
the already-wrapped functions), so their stats dumps contained all data
the parent had accumulated before the fork. The parent dumps that same
data itself, and gather_stats() merges by summation, so every line
executed before a fork was counted once per forked child: a 100k-loop
parent with three fork children reported 400,004 hits instead of
100,001, for hits and times alike. fork is the default start method on
Linux <= 3.13, so merged --prof-child-procs numbers were wrong in the
most common configuration. (Evidence: dev/planning/
fable-review-pr431-2026-07-05.md, finding P1.)

Fix: snapshot the profiler's stats at the top of the os.fork wrapper's
child branch and pass them down to _DumpStatsHelper as a subtractive
baseline; every dump now writes get_stats() - baseline via the new
LineStats.__sub__/__isub__ (the exact inverse of __add__, raising
ValueError when the subtrahend is not a prefix of the minuend). The
delta is re-derived on every dump since pool workers dump per task.
Nested forks fall out naturally: each fork snapshots the state at its
own fork point.

New tests: tests/test_child_procs/test_fork_stats_attribution.py runs
the real CLI in a real subprocess over fork/spawn/forkserver x
Process/Pool/ProcessPoolExecutor and asserts exact hit counts, plus a
multi-fork variant asserting counts do not scale with child count.
(These also add the first ProcessPoolExecutor coverage.) LineStats
subtraction is covered by doctests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mandatory pool patch changes the parent<->worker result protocol
(workers push their PID with each result), and the patched parent
unconditionally 2-way-unpacked every result. A worker that was never
patched -- e.g. its interpreter never loaded the profiling .pth hook,
or multiprocessing.set_executable() points at a different Python --
pushes vanilla 3-tuples, so the unpack raised ValueError inside the
pool's result-handler thread; the thread died and every
AsyncResult.get() blocked forever. --prof-child-procs turned a working
program into a deadlock (reproduced; evidence: dev/planning/
fable-review-pr431-2026-07-05.md, finding P2), and load_pth_hook
swallowing all exceptions made every silent hook failure take this
path.

Fix:
- Tagged protocol: patched workers now push (PID_TAG, pid, obj)
  triplets, where PID_TAG is a distinctive module constant.
- Tolerant parent: _wrap_outqueue_quick_get() strips tagged results
  and passes anything else through untouched with a once-per-session
  UserWarning that the worker's profile data will be missing.
- Loud hook failure: load_pth_hook() now always warns when child
  setup fails (previously only under DEBUG) while still letting the
  child run unprofiled.

Tests: unit tests for the tagging/unwrapping, an integration test
running a patched parent against genuinely vanilla spawn workers
(hard-timeout-guarded: a regression fails in seconds instead of
hanging CI), and a fork control case asserting no warning when the
workers are patched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both call_callback() and set_local_trace() stored the wrapper objects
they create (via disable_line_events() / wrap_local_f_trace()) on
frame.f_trace with PyObject_SetAttrString(), which takes its own
reference, while never releasing the creation reference. One wrapper
leaked per wrapped trace event -- unbounded growth on long profiled
runs alongside a debugger or coverage tool. The in-code comment
claiming nothing else holds a reference was wrong once SetAttr
succeeds. (Evidence: dev/planning/fable-review-fullrepo-2026-07-05.md,
finding F8; empirically confirmed -- the wrappers survived deletion of
every Python reference.)

Also fixed in set_local_trace():
- a NULL call result was passed to PyObject_SetAttrString, turning it
  into an attribute delete executed with a live exception set (C-API
  misuse);
- failures now report via PyErr_WriteUnraisable instead of leaving an
  exception set that the void/no-except Cython call site never checks;
- PyUnicode_FromString result is NULL-checked;
- the direct f_trace assignment uses Py_XSETREF (no longer overwrites
  a possible Py_None without release).

Removed the dead mod/dle locals in call_callback().

Regression test: capture frame.f_trace wrappers via weakref from
inside a profiled function under a foreign trace callback (including
one that disables f_trace_lines), then assert they die with their
frames. Runs under LINE_PROFILER_CORE=legacy in a subprocess; fails
against the previous .so, passes after this fix. Note the wrappers
masquerade as the functions they wrap (@wraps copies __qualname__), so
weakref-death is the only reliable observable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…R-1)

The sys.monitoring backend kept enable/disable state in per-thread
_LineProfilerManager objects while set_events()/free_tool_id() act
process-globally. As soon as the first-registering thread's manager ran
out of active profilers it tore down the global events and freed the
tool, silently ending data collection for every other thread and making
their later disable() calls raise 'ValueError: tool 2 is not in use' --
which surfaces inside user code, since @Profile wrappers disable in a
finally block. 3.12+ defaults to this backend. (Evidence: dev/planning/
fable-review-fullrepo-2026-07-05.md, finding F1.)

Fix: one _SysMonitoringState is now shared per tool id
(_get_shared_mon_state()), and in sysmon mode every manager's
active_instances aliases the shared state's set, so the existing
enable/disable logic becomes globally correct: the global callbacks
are registered when the first profiler anywhere enables and torn down
when the last one anywhere disables, regardless of which thread does
either. A bare process-wide refcount (the originally-planned fix)
would not have sufficed: attribution consults the handling manager's
active_instances, so a profiler enabled on another thread would have
recorded nothing.

Also: deregister() is now idempotent (no-op unless registered) and
tolerant of an externally-freed tool; disable() clears the in-progress
line bookkeeping for all threads under sysmon (still recreating the
caller's empty entry, which c_last_time expects). The legacy trace
core keeps its strictly per-thread state and semantics.

Regression test: tests/test_cross_thread_profiling.py runs two
profilers on two threads with the first enabler bowing out mid-flight,
under both cores in subprocesses. Before this fix the sysmon variant
lost thread B's remaining hits and raised ValueError; both variants
now assert the exact 2xN hit count. Full test suite passes (429
passed, 1 skipped, 1 xfailed).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… commits

Status boards and agent logs updated with what was done, deviations
from the specs, and gotchas for future agents (CallbackRepr recursion,
dump_stats log-grep coupling, @wraps qualname masquerade, why a bare
refcount was insufficient for FR-1).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@TTsangSC

TTsangSC commented Jul 6, 2026

Copy link
Copy Markdown
Owner

(Oh crud, I accidentally last-paged when in the middle of typing something here. Gotta do it all the dang way again...)

Thanks for the input; several things:

General

  • Just out of curiosity: I noticed that Fable1 seems to be comparing between the current and previous iterations of the PR. Does it mean it had to crunch through the entire commit history of the branch?
  • In .llm_resource_tally/tool/version.py you've implied that the entire vendored-in copy of llm_resource_tally is supposed to be committed into the repo anyway,2 but I wonder if it makes sense to just keep the ledger and lifetime-totals.json and .gitignore the rest? Since at some point user intervention is needed to opt in and set up the commit hooks anyways – albeit with a mere python <path-to-llm_...> install – maybe the downloading and installation of llm_resource_tally could've been left inside a dev/ script?

Fable full-repo review

  • F-1:
    • So we're moving to more thread-shared states... guess that is fine since we already don't have no-GIL guarantees.
    • That probably means _threading_patches.py will be unnecessary because the entire rationale of that module is to sync the .enable_count of the profiler between the spawning and the spawned threads.
    • Same goes for _test_child_procs_utils.py::_cleanup_profiling_in_current_thread() which is only a hack for cleaning up profilers created in other threads.
  • F-5:
    So basically autoprofile cannot handle multiple imports on the same line pyutils/line_profiler#433. Mind giving FIX: handling multi-target (from-)import statements pyutils/line_profiler#434 a look?
  • F-6:
    Not sure if I agree with the analysis here.
    1. Normalizing to the coarsest .unit ensures that we don't over-report on precision.
    2. Because .unit is directly proportional to the normalizing factors, sorting on .unit helps with preserving numerical stability.
    3. The normalized timing figures are floats, are accumulated as such, and are only converted to ints at the very end of LineStats._get_aggregated_timings(). So the assertion that the normalizing scheme "does the opposite" to minimizing errors is unfounded, at least if we want to stick to (1).
  • F-11:
    I wonder if we can just take a page from the PR (runpy_patches.py::create_runpy_wrapper()), and create a patched copy of inspect with _CythonBlockFinder to be used in line_profiler.py.

Fable PR review

  • P1:
    Good catch, should've written a test case for that.
  • P2:
    • Yikes, wasn't aware of BaseContext.set_executable(). In that case tagging the communicated tuple and falling back to the vanilla behavior do seem ideal.
    • It may be worth looking at something like coverage.multiproc.Stowaway, which results in the .pth hook being executed when the child process unpickles the input, instead of at interpreter startup via the .pth file. I originally had that in the code, but eventually decided to trim that away since the setup "must have" already happened via .pth anyway. Seems that there might be cases where that isn't the case...
  • P3:
    • Ah nice, was wondering if sysconfig works for non-venv installs.
    • The non-installation caveat may require some hacky solution for cases where _line_profiler_hooks.py isn't import-able, since the .pth always have to be a one-liner beginning with import. Maybe something like
      import importlib.util as iu; spec.loader.exec_module(mod) if (mod := (iu.module_from_spec(spec) if (spec := iu.find_spec('_line_profiler_hooks')) else None)) else None; getattr(mod, 'load_pth_hook', lambda ppid: None)(PPID)
  • P7:
    Yeah the plan was to get the code there and the components stable first because actually documenting them. But it makes sense to have that in the roadmap.
  • P8:
    I basically just wrote the entry-point (plugin) system to sidestep having (potetntially circular) imports; exposing the entry point is more of a side effect. Guess that it does make sense, incurs less overhead, and is more explicit to just suck that up and shove the imports into the body of the instantiatior... and we can just get rid of the entry-point stuff entirely.
  • P10:
    The doubled-up warning is intentional because Logger.warning() messages may be suppressed depending on the kernprof [--verbose|--quiet [...]] setting. (See line_profiler/_logger.py::Logger.configure() and kernprof.py::_parse_arguments().)
  • P11:
    • As noted in the previous discussion (and evident from the long and arduous strugglewith SIGTERM), I'm not sure if there's a way to ensure result correctness without a per-task stats dump.
    • Maybe pickling the stats and pushing it back to the parent (like what we already do for PIDs) would reduce the per-task overhead compared to writing the stats to disk.
  • P13:
    I doubt if instantiating a new profiler instance tempers with tool registration, since that is only done in line_profiler/_line_profiler.pyx::_SysMonitoringState.register(), which is only invoked if the profiler is enabled. But yeah it's probably unnecessary, and if we're updating the Cython code anyway we might as well expose a Python-level hook to timers.c::hpTimerUnit().
  • D.2, D.3:
    Good catch.
  • D.6:
    os.environ cleanup is tested in test_child_procs.py::test_cache_dump_load(), but I guess that point the model is making is that it isn't tested to be guaranteed "in prod" because of e.g. the failing edge cases of kernprof.py::_manage_profiler.
  • D.10:
    • Yeah Params.__mul__() is functionally equivalent to stacking @pytest.mark.parametrize decorators, but the point is to enable Params.__add__() which takes the default for the unlisted parameters and adds to them the listed ones. But one can maybe argue that instead of (ab-)using parametrizations it'd be clearer to split the cases into separate test functions.
    • The problems with pytest.warns are that:
      • Each such context manager can only check for a single warning specification.
      • It doesn't allow for checking against warnings. One can maybe use warnings.check_warnings() and warnings.simplefilter('error', ...) for that, but then the warnings are raised at the point of issuing, instead of being checked post-hoc and resulting in a ResultMismatch which documents what is wrong in the error message. (But then course one can also argue that the former behavior is more direct and thus better.)
  • D.11:
    Yeah that one's weird. For some unholy reasons the test works with pytest --doctest-modules but not pytest --xdoctest... even more curiously, both python -m doctest and python -m xdoctest works when said file is put on the path via ${PYTHONPATH}. So I assume that to be because of maybe differing handling of the sys.path injected by pytest between the two doctest backends. I did try for quite a while to debug that before settling with the SKIP, intending to write a PR to xdoctest if needs be, but failed to locate the cause before giving up.

Footnotes

  1. Hold up, am I even supposed to be looking at this at all? My being a non-American citizen and all... /jk

  2. This also resulted in a somewhat awkward situation where Erotemic/llm_resource_tally contains two copies of the source code.

@Erotemic

Erotemic commented Jul 6, 2026

Copy link
Copy Markdown
Author

Oh crud, I accidentally last-paged when in the middle of typing something here. Gotta do it all the dang way again

That's why I type almost everything out in vim first :)

comparing between the current and previous iterations of the PR.

I gave it the context of the PR conversation. But I had your latest branch checked out when I ran the prompt. It could have read the entire history, but that wasn't my intent.

entire vendored-in copy of llm_resource_tally

Yeah, I'm reconsidering that choice. It seems like its easy enough for it to install itself .gitignored, technically its only installing a "minimal" version of itself, which doesn't include all the carbon modeling, but after looking at this PR it hit me how much bloat it was. Of course, after you commit it once, it never shows up in the diff again, and having a note in AGENTS.md and the code available means its more likely that if anyone uses an LLM to make a contribution, the resources get tracked.

F-5: Mind giving FIX: handling multi-target (from-)import statements pyutils#434 a look?

I'll make some time for it. I wanted to land this first. Preference on order?

F-6: Not sure if I agree with the analysis here.

I mean technically its right, in that you are losing the details, but it may just be overreading the intent of the doc. I agree it's a non-issue.

F-11: create a patched copy of inspect with _CythonBlockFinder to be used in line_profiler.py.

A bit more maintainer burden, but I'm not opposed to it.

P11

The remediation is just "| PR431-11 | Document (and later maybe throttle) per-task flush cost | small | — | todo |"?

Probably a good idea to document, but would throttling help in the sense that
the results would still be correct? Or would a throttle drop measurements? I
guess the more important question is how big do the stats get? And did we have
to effectively have to pay this cost before this patch for the main thread? So
now its just multipled by the number of workers? If so that seems acceptable,
but if this is a new cost that would degrade performance of existing users on
their main thread profiles when they have multiprocessing happening, then we
may need to think about this some more.

This reminds me I need to start branchmarking this tool more rigorously and
tracking regressions. If we had that it would be pretty easy to get a feel for
what the real cost of this is.

PR431-11

unholy reasons the test works with pytest --doctest-modules but not pytest --xdoctest... even more curiously, both python -m doctest and python -m xdoctest works when said file is put on the path via ${PYTHONPATH}.

The pytest integration in xdoctest is extremely old at this point. In part for backwards compatability, but also in part because it still works. I wonder if it would help if I modernized it. I could try to look into that.

Hold up, am I even supposed to be looking at this at all? My being a non-American citizen and all... /jk

Don't get me started ;)

This also resulted in a somewhat awkward situation where Erotemic/llm_resource_tally contains two copies of the source code.

100% intentional. There's a lot in the design I'm still questioning / considering, but I'm convinced that the self-replicating install is the right move for the tool.

@TTsangSC

TTsangSC commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Replies

type almost everything out in vim first

Yeah lesson learned, that's what I ended up doing for the rewritten reply.

I wanted to land this first. Preference on order?

Personally I'd say it may be a good idea to merge small and easy fixes for correctness first, but of course if one was to always do that big features would never land. In that case I trust your judgment.

would throttling help in the sense that the results would still be correct

Um... can you clarify what you meant by throttling? Like, only writing stats every n tasks or something? (Probably not, since a per-task hook to do whatever is the only way to ensure that things get done in workers before control is passed back to the parent, which may axe it at any point.)

I've also realized that it's probably strictly easier and more performant to just go with sending the stats alongside result since we have to pickle the LineStats either way.

But yes of course it makes sense to start with documenting the overhead in any case.

did we have to effectively have to pay this cost before this patch for the main thread

In the main process stats there're no tasks to speak of and writing only happens at the end, so probably no both pre and post this this PR.

if this is a new cost that would degrade performance of existing users on their main thread profiles when they have multiprocessing happening

The changes made by the PR is entirely opt-in via --prof-child-procs, and none of patching stuff happens unless said flag/config option is set (default false; see line_profiler/rc/line_profiler.toml::[tool.line_profiler.kernprof] and kernprof.py::_manage_profiler.__enter__()) – with the singular exception of line_profiler/_threading_patches.py which deals with enabling the profiler in a new thread, and is automatically applied by line_profiler/curated_profiling.py::CuratedProfilerContext. So there shouldn't be appreciable changes in performance unless one were to actually use the feature.

Even if a user was to use multiprocessing.dummy with kernprof --prof-child-procs, we still won't incur per-task pickling overhead, because stats are only dumped when the cache object has a ._stats_dumper – which is not the case in the main process.

pytest integration in xdoctest [...] I could try to look into that

Might have figured that out. sys.path manipulation should've happened via _pytest.pathlib.import_path(), which is in turn called by _pytest.python.importtestmodule(), _pytest.python.Module._getobj(),Module.obj, various internal methods in Module, which finally ends up being called by Module.collect(). Since your XDoctestTextfile.collect() doesn't interact with the base-class implementation, we never accessed .obj and thus the path patching didn't happen.

A naïve fix is simple (just access self.obj in your .collect()), but a caveat is that when pytest runs and all that happens, sys.path is altered with no provisions for restoration (except when it's e.g. effected by monkeypatch):1

>>> import pytest
>>> sp = sys.path.copy()
>>> pytest.main(['--doctest-modules', '-k', 'Worker', 'tests/test_child_procs'])  # Doctest in question
...
<ExitCode.OK: 0>
>>> set(sys.path) - set(sp)
{'<...>/tests/test_child_procs/multiproc_examples', '<...>/tests'}

which IDK if it's acceptable to xdoctest, given the emphasis on being contained and getting things done with static analysis when it can be helped. But then again, if we're already interfacing with pytest we might as well lean into it; and xdoctest.plugin doesn't affect how the package functions either, when not used with pytest.

I'll write an issue and maybe a PR at xdoctest. In the meantime of course I can also fix Worker.__enter__() and Worker.__exit__() to handle the sys.path stuff.

Apropos, when writing the doctests I did occasionally run into pytest INTERNALERRORs with cryptic messages, usually when the code is slated to be on the unhappy path and when either:

  • The doctest is backslash-continued, or
  • The exception name in the Traceback (most recent call last):\n ...\n<xc name>: <xc message> isn't an exact match.

A bit harder to isolate those though, will ping you on that repo should I get round and make progress on that.

Other notes

llm_resource_tally notwithstanding, it still seems that we have quite a lot of heavy stuff here which are better served by separate PRs to trunk here – especially FR-1. Should I merge the whole PR nonetheless, or do you think I can just leave it open and cherry-pick the commits directly fixing pyutils#431?

Footnotes

  1. Seems to be a known issue (pytest pollute sys.path,  pytest-dev/pytest#9219) that they have no intent to fix. Wondering why, doing a try-finally in pytest.main() and restoring sys.path seems like such an intuitive thing to do... but then again I guess one can argue who's to say it hasn't been changed intentionally by the test code and pytest.main() enforcing a restoration of sys.path would've been overreach.

@TTsangSC

TTsangSC commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Also since we're already looking at import-rewrite bugs (fable-review-fullrepo-*.md::F4 and F5), it may be worth keeping in the back of our minds to check if lazy imports work once 3.15rc1 is out. (But then we'll probably have bigger fish to fry with the C API; IDK though, haven't looked too closely.)

@TTsangSC

TTsangSC commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Also noticed a problem with P12: there doesn't seem to be a portable and Python-native way to check which PIDs are active. The os functions which take PIDs requires said PIDs to be of children of the calling process, and as such are useless for inferring which .pth files are likely to be in use by other Python processes.

I can always cobble something together with ps like

import os
import subprocess
from collections.abc import Callable, Collection
from shutil import which


get_active_processes: Callable[[], set[int]]


def _find_executable(name: str) -> str:
    maybe_path = which(name)
    if maybe_path:
        return maybe_path
    raise RuntimeError(f'cannot find required executable for `{name}`')


def processes_are_alive(pids: Collection[int]) -> dict[int, bool]:
    all_pids = get_active_processes()
    return {p: p in all_pids for p in set(pids)}


def _get_active_processes_posix() -> set[int]:
    # `a`: processes from all users / `x`: include detached processes
    # Note that we don't do `aux` because we don't want the user clogging up the 1st column
    cmd = [_find_executable('ps'), 'ax']
    ps = subprocess.run(cmd, capture_output=True, text=True, check=True)
    header, *procs = ps.stdout.splitlines()
    assert header.strip().split()[0].lower() == 'pid', header
    return {int(p.strip().split()[0]) for p in procs}

But I'm really not sure about tasklist, Microsoft's docs aren't being all that helpful and I don't have a Windows machine to test. Maybe something like... ?

from csv import DictReader


def _get_active_processes_windows() -> set[int]:
    # CSV formatted output
    cmd = [_find_executable('tasklist'), '/fo', 'csv']
    tasklist = subprocess.run(cmd, capture_output=True, text=True, check=True)
    header, *rows = tasklist.stdout.splitlines()
    headers = {h.strip() for h in header.lower().split(',')}
    assert 'pid' in headers, headers
    return {int(row['pid']) for row in DictReader([header.lower(), *rows])}


if os.name == 'posix':
    get_active_processes = _get_active_processes_posix
elif os.name == 'nt':
    get_active_processes = _get_active_processes_windows
else:
    raise RuntimeError(f'unsupported platform `{os.name}`')

@TTsangSC

Copy link
Copy Markdown
Owner

More notes:

P3

P3 (HIGH, blocks merge): unwritable site-packages hard-crashes kernprof

[...]
Remediation: task PR431-3 in the plan (fallback dirs, warn-and-degrade).

Done (will push shortly), but then I realized that there probably isn't a good way for us to test that. Maybe a POSIX-only test where we chmod u-w <pure-lib dir>?

D.7

Patch-reversal verification is self-referential: tests assert restoration against the product's own Patch.summary metadata (_test_child_procs_utils.py:1594-1757), so an undeclared mutation is invisible to both sides.

IDK if there's a meaningful way to fix this; we're reliant on trusting either the programmatic declarations on Patch.summary (which insofar as SingleModulePatchs are used are guaranteed to be correct, since they are constructed with the .add_target() and .add_method() methods which automatically maintain the .summary), or maintainers to correctly update a manually-constructed table of changes every time changes are made to line_profiler._child_process_profiling. But if it bolsters confidence I can always add an explicit table of patches somewhere in _test_child_procs_utils.py and an assertion that it ≥ PATCH_SUMMARIES['maximal'].

@Erotemic

Copy link
Copy Markdown
Author

We don't have to hit everything fable identified, just the meaningful ones.

I'm also thinking I'm not going to merge the llm-resource-tally into line-profiler directly. I have it working now so it can install as an invisible-to-git hook, and it might be better to do any tracking outside of the repo itself. Given that this repo is mature the llm usage related to it will be much lighter than projects that are entirely vibe coded, which is more of what the order of magnitude estimations it provides are useful for. For this project the measurements may be too noisy to say anything meaningful. Also the fact that any interaction I have with a chat-based service doesn't show up on it, and that is still meaningful for advancing the project - especially with GPT 5.6 out now - is something that bothers me, and I don't know how to track it well. Maybe it makes sense to tie LLM usage to the developer rather than to the repo, and I can track - and ideally mitigate - how much my own carbon footprint has increased because of my usage.

The upshot: I think this branch lives as a reference and we can cherry pick or rework any pieces of it we need into your PR or into new PRs.

Done (will push shortly), but then I realized that there probably isn't a good way for us to test that. Maybe a POSIX-only test where we chmod u-w ?

Seems doable. Having the mitigation is is more important, while it is nice, we don't have to test every corner case. But if a u-w chmod works on a posix-only test, then that's a good idea.

D.7

One thing I've noticed about LLMs is that they are overeager to build "ratchet" or "test-the-test", or "test-code-shape" tests that traditionally humans wouldn't bother to write. At first I told LLMs I was working with to stop doing it, because it seemed wasteful to me, but if you are doing a lot of agentic coding it actually does help, because it prevents the LLMs from going off the rails and undoing a previous design decision that you made. At the same time I've also seen it make bad design decisions and enforce them in tests, and then I had to argue with them to remove them and refactor. That being said, I think D.7 is safe to ignore. This isn't a project where an LLM will be able to commit anything without having it manually reviewed.

@TTsangSC

Copy link
Copy Markdown
Owner

Update: I'm more or less on the final stretch (code-wise; docs await) here except with one hiccup: concurrent.futures.

  • Despite the name, ProcessPoolExecutor doesn't use a multiprocessing.pool.Pool, instead implementing similar machineries that manages multiprocessing.process.BaseProcess instances... which means that it only really has the multiprocessing.process patch, and should theoretically suffer from inconsistencies introduced by e.g. Process.terminate() like an unpatched Pool would, esp. if the parallel workload fails. But it doesn't,1 and that's what worries me.
  • Apparently Process.terminate() is only called in limited places: either explicitly via e.g. ProcessPoolExecutor.terminate_workers(), or in _ExecutorManagerThread.terminate_broken() which is run when the executor is "in a broken state" – whatever that means. So now we'll need to figure out a way to meaningfully break PPE to test any hypothetical patch for concurrent.futures. (The patch itself should be the easy part though, since we can port what we already have for multiprocessing.pool over for PPE, because they both work on a task basis.)
  • Another surprising part is that I do get test failures with ThreadPoolExecutor, the equivalent of multiprocessing.dummy, which I thought line_profiler._threading_patches would've fixed. (But apparently not, since the full-repo review also had some choice words about TPE.) Will check if it's fixed with c3b2915.

Footnotes

  1. Whipped up a test script which basically does the same thing as the other test modules in tests/test_child_procs/multiproc_examples and ran it through the wringer (since we're already set up in test_child_procs.py to just take whatever new test module as a fixture and run it); so far nothing has broken... as long as we're using PPE (multiprocessing-backed) and not TPE (threading-backed) – see the last bullet point.

TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-review-pr431-2026-07-05.md::D.11` in #5)

tests/test_child_procs/multiproc_examples/process_test_module.py::Worker
    __doc__
        Removed `# xdoctest: +SKIP` directive
    .__enter__(), .__exit__()
        Now temporarily patching the module's parent directory into
        `sys.path` to ensure that `Worker` and its members can be
        pickled by `multiprocessing`; see `xdoctest` issue pyutils#207
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-review-pr431-2026-07-05.md::P9`, `P13`, `D.2` in #5)

<Various files>
    Fixed typoes in docs and comments

_line_profiler_hooks.py::load_pth_hook()
    Moved load-once check to before the imports (overhead of
    re-importing already-imported modules should be minimal, but anyway)

line_profiler/line_profiler_utils.py::make_tempfile()
    Now erroring out and removing the tempfile if we can't close the
    handle created by `tempfile.mkstemp()`

line_profiler/_child_process_profiling/multiprocessing_patches
/__init__.py::apply()
    Removed outdated references to Windows-specific behaviors

tests/test_child_procs/test_child_procs.py
::_test_apply_mp_patches_inner()
    Removed outdated config for polling in
    `line_profiler._child_process_profiling.multiprocessing_patches`
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-review-pr431-2026-07-05.md::P13` in #5)

line_profiler/_child_process_profiling/_cache_logging.py
    TIMESTAMP_PATTERN
        Added formatting field `level`
    add_timestamp()
        Added optional argument `level`
    CacheLoggingEntry
        .__doc__
            Updated instantiations in doctest
        .level
            New field (2nd item)
        .to_text(), .from_text(), .write()
            Added handling of the `.level` field
        .new()
            Added optional argument `level`

line_profiler/_child_process_profiling/cache.py::LineProfilingCache
    _debug_output()
        - Added optional argument `level` in keeping with the base class
        - Moved implementation of `._make_debug_entry` (used nowhere
          else) inside
    _setup_in_child_process()
        Updated patching of `CuratedProfilerContext._debug_output`
    _make_debug_entry
        Removed

line_profiler/cleanup.py::Cleanup
    _debug_output()
        Added optional argument `level` for setting the logging level
    _cleanup()
        If a callback fails, it is now logged with `Logger.warning()`
        instead of `Logger.debug()`
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-review-pr431-2026-07-05.md::P10` in #5)

line_profiler/line_profiler.py::LineStats.from_files()
    - Added new private params `_note_on_empty` and `_note_on_defective`
      so that calling methods can bolster the emitted warning messages
      with relevant notes
    - Improved formatting for errors without arguments in warnings
    - Fixed formatting of the empty-file warning messages
    - Reworded defective-file warning messages

line_profiler/_child_process_profiling/cache.py
::LineProfilingCache.gather_stats()
    Now calling `LineStats.from_files()` with notes for more helpful
    warning messages
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-review-pr431-2026-07-05.md::P3` and `P12` in #5)

line_profiler/_child_process_profiling/_retrieve_pids.py
    New helper submodule for checking which PIDs are active

    Caveats:
    - There is no Python-native method for checking the process tables
      so the implementations are platform-specific and relies on parsing
      command output (`ps` on POSIX, `tasklist` on Windows)
    - Said parsing esp. on Windows may be locale-sensitive
    - TODO: we haven't even tested whether Windows implemention works
      yet...

line_profiler/_child_process_profiling/cache.py::LineProfilingCache
    _setup_in_main_process()
        Now allowing for `.write_pth_hook()` to error out, emitting a
        warning that profiling cannot continue in children except for
        forked ones
    write_pth_hook()
        - Added param `clean_stale` to optionally run cleanup for .pth
          files associated with stale main PIDs; during normal execution
          they should've been cleaned up by the owning process, but if
          it is e.g. killed this may be necessary to prevent .pth files
          from piling up
        - Now including info about the main PID in the filename of the
          .pth file, along with other normalizations of the affixes;
          note that this means the filename stem is no longer
          guaranteed to start with `prefix` and end with `suffix`
        - Added search order for the default `dir` to write the .pth
          file to:
          - Location of `_line_profiler_hooks` if a `site-packages`
            directory
          - `sysconfig.get_path('purelib')` (existing behavior)
          - `site.getusersitepackages()` if `site.ENABLE_USER_SITE` is
            true
          The .pth file is written to the first writable location
        - The .pth file written now fails gracefully in case the
          function (`_line_profiler_hooks.load_pth_hook()`) to be
          imported and called cannot be found for whatever reason
    _cleanup_stale_pth_files(), _find_pth_files()
        New helper methods for locating and pruning .pth files
        associated with stale PIDs
    _get_graceful_oneline_call()
        New (doctest-ed) helper method for writing a .pth file calling a
        function, which gracefully/silently fails in case of
        module/function lookup errors; this prevents .pth files
        installed to shared locations from tripping up other
        interpreters, esp. those without `_line_profiler_hooks`
        installed
    _enumerate_pth_installation_locations()
        New helper method for enumerating the possible locations to
        install a .pth file to

line_profiler/rc/line_profiler.toml
::[tool.line_profiler.child_processes.pth_files]
    Updated docs
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-pr431-plan-2026-07-05.md::P8` in #5)

line_profiler/_child_process_profiling/multiprocessing_patches
/__init__.py
    _PATCHES
        Now loaded from `Registry.get_default()`
    apply()
        Replaced assertion with `RuntimeError`

line_profiler/_child_process_profiling/multiprocessing_patches
/_infrastructure.py::Registry
    DEFAULT_ENTRY_POINT, from_entry_point()
        Removed (we don't actually expect to load patches from 3rd party
        packages, so just hard-code the patch locations)
    get_default()
        New instantiator refactored from `.from_entry_point()` loading
        patches from the hard-coded sibling submodules

setup.py
    Removed the now-unused `line_profiler._multiproc_patches` entry
    point
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-review-pr431-2026-07-05.md::P4` in #5)

line_profiler/_child_process_profiling/multiprocessing_patches
/_mandatory_patches.py
    wrap_join_exited_workers()
        New wrapper around `Pool._join_exited_workers()` which mirrors
        the bookkeeping that `wrap_terminate_pool()` does for workers,
        except instead it is for that exited early during the pool's
        lifetime
    wrap_terminate_pool()
        Replaced assertion in the `finally` clause with a warning that
        some worker processes remains alive after calling
        `Pool.terminate()`
    RebootForkserverPatch.reboot()
        Rewrote bare assertion to be more informative (shouldn't happen
        though)
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-pr431-plan-2026-07-05.md::P11` in #5)

line_profiler/_child_process_profiling/cache.py
    _StatsHelper
        - Refactored from `_DumpStatsHelper`
        - Added new methods `.get()` and `.dump()`, paralleling
          `LineProfiler.get_stats()` and `.dump_stats()`
    LineProfilingCache._stats_helper
        - Renamed from `._stats_dumper`
        - Now a `_StatsHelper | None`

line_profiler/_child_process_profiling/multiprocessing_patches
/__init__.py::apply.__doc__
line_profiler/rc/line_profiler.toml
::[tool.line_profiler.child_processes.multiprocessing.patches]
    Updated descriptions of the `pool` patch

line_profiler/_child_process_profiling/multiprocessing_patches
/_mandatory_patches.py
    wrap_terminate_pool(), wrap_join_exited_workers()
        Removed (refactored into
        `.._pool_patch_helpers.get_worker_finalization_patch()`)
    POOL_WORKER_PID_PATCH
        - Updated data-processing callback to be compatible with
          `get_per_task_callback_patch()`, and to emit debug-log
          messages for the # tasks handled by the worker
        - Now also constructed with `get_worker_finalization_patch()`

line_profiler/_child_process_profiling/multiprocessing_patches
/_pool_patch_helpers.py
    get_per_task_callback_patch()
        - Refactored from `.._queue.get_mp_pool_patch()`
        - Updarted call signature of `get_data`:
          `(LineProfilingCache) -> T`
        - Added optional param `patch` so that we can stack the
          `get_*_patch()` functions
    get_worker_finalization_patch()
        New helper function Rrefactored from
        `.._mandatory_patches.wrap_terminate_pool()` and
        `.wrap_join_exited_workers()`, returning a single
        `SingleModulePatch` object patching the resp. methods to execute
        callbacks on finalized worker processes

line_profiler/_child_process_profiling/multiprocessing_patches
/_profiling_patches.py
    wrap_worker()
        Removed
    POOL_PATCH
        - Reworked functionality: instead of writing the accumulated
          stats to disk on each task processed, worker child processes
          now send the stats back to the parent process, which is then
          responsible for the writing when the worker objects are
          "finalized" (joined or terminated); this should reduce the
          per-task overhead, particularly in terms of IO
        - Now also constructed with `get_worker_finalization_patch()`

line_profiler/_child_process_profiling/multiprocessing_patches
/_queue.py::get_mp_pool_patch()
    Removed (superseded by
    `.._pool_patch_helpers.get_per_task_callback_patch()`)

tests/test_child_procs/test_child_procs.py
::test_cache_setup_child(), _test_apply_mp_patches_inner()
    Updated debug-log messages to be matched, because the writing of
    profiling stats is no longer necessarily a result of a
    `Cleanup.cleanup()` call
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-pr431-plan-2026-07-05.md::P5` and `P6` in #5)

kernprof.py::_manage_profiler
    .__enter__()
        Fixed bug where if the setups for the executed script and/or the
        session cache fails, the `CuratedProfilerContext` isn't cleaned
        up
    .__exit__()
        Fixed bug where if the gathering of child-process profiling
        stats fails, the process-local stats are not written
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-pr431-plan-2026-07-05.md::D.1`, `D.3`, `D.4`, `D.8–.10` in
#5)

tests/test_child_procs/_test_child_procs_utils.py
    FuncCallTimeout
        Renamed from `TestTimeout` so that `pytest` doesn't try to
        collect it as a unit-test class and emit a warning
    _run_kernprof_main_in_process()
        Now accepting `cmd` which begins with e.g.
        `['python', '-m', 'kernprof']` and other similar forms, instead
        of only `['kernprof']` (`D.1`)
    @cleanup_extra_pth_files
        - Refactored to only process .pth files which is consistent with
          the `LineProfilingCache.write_pth_hook()` naming scheme
          (`D.3`)
        - Added doctest
    @preserve_object_attrs
        Now emitting debug messages for teardown-time errors and
        re-raising the last (`D.9`)
    ResultMismatch.rich_message
        Removed unused property (`D.10`)
    CheckWarnings
        - Extended doctest to cover `.suppress_warnings()` and
          `.propagate_warnings()`, because it makes more sense to keep
          both methods for symmetry (`D.10`)
        - Removed implementations of `Sequence` mixin methods (the
          inherited impls are more inefficient, but we don't really use
          any of them in the code anyway) (`D.10`)

tests/test_child_procs/conftest.py
    check_purelib_dir_writable()
        New fixture to be requested by tests, which tests for the
        writability of `sysconfig.get_path('purelib')` and skips the
        test if it isn't (`D.4`)
    another_pid()
        Replaced a (botched) subtract-and-mod with a bitwise XOR to
        ensure that the "PID" is both different and positive (`D.8`)

tests/test_child_procs/test_child_procs.py
    test_running_multiproc_script()
    _test_profiling_multiproc_script()
    _test_profiling_bare_python()
        Replaced unqualified `kernprof` commands with
        `<sys.executable> -m kernprof` (`D.1`)
    test_cache_setup_main_process()
    test_apply_mp_patches_{success,failure}()
    test_profiling_multiproc_script_{success,failure}()
    test_profiling_bare_python_{success,failure}()
        Now requesting `check_purelib_dir_writable`, skipping the tests
        when the pure-lib path directory is non-writable (`D.4`)

tests/test_child_procs/test_fork_stats_attribution.py
    - Added type annotations
    - Refactored to avoid awkward indentations (which trip up the syntax
      highlighter)
    - Tests now requesting `check_purelib_dir_writable`, skipping the
      tests when the pure-lib path directory is non-writable (`D.4`)
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-pr431-plan-2026-07-05.md::D.5-6` in #5)

kernprof.py
    _write_preimports()
        Now returning `None` if the tempfile has been immediately used
        and deleted
    main(), _manage_profiler.__exit__()
        Added checks to file-cleanup/IO routines so that they are no-ops
        if we're in a forked child created inside the executed code; all
        such interactions with the "outside world" should be handled by
        the calling parent process

tests/test_child_procs/_test_child_procs_utils.py
    preserve_targets
        .__doc__
            Extended doctest to test `verify`
        .__init__()
            Added parameter `verify`
        .__enter__(), .__exit__()
            Updated implementation to store the dict of old values and
            compare them with the values at context-exit with
            `.compare_with_current_values()` if `.verify` is true
    _run_kernprof_main_in_process()
        - Updated decorator: `@preserve_targets(verify=True)`
        - Added check against `os.environ` pollution (`D.6`)

tests/test_child_procs/test_child_procs.py
    _test_applymp_patches_inner()
        Updated decorator: `@preserve_targets(verify=True)`
    test_profiling_multiproc_script_{success,failure}()
        Reworked parametrization to:
        - Remove unintentional default (`start_method=None`) cases
        - Ensure that we run at least one subtest with
          `subprocess.run(['kernprof', ...])` for each combination of
          `start_method` and `fail` (`D.5`)
    test_profiling_bare_python_{success,failure}()
        - Updated call signature
        - Added subtests which test when the new Python process is
          created via `os.fork()` (`D.6`)
TTsangSC added a commit that referenced this pull request Jul 13, 2026
(See `fable-review-pr431-2026-07-05.md::D.6` in #5)

tests/test_child_procs/_test_child_procs_utils.py
::check_tagged_line_nhits()
    Added new optional parameter `comparator` so that comparisons other
    than `operator.eq` can be done

tests/test_child_procs/test_abnormal_execution.py
    New test module for various edge cases (`D.6`)

    test_prematurely_terminated_process()
        Test that if a process is `.terminate()`-ed, it impairs
        collecting profiling data therein but doesn't affect the rest of
        the profiling session
    test_corrupted_child_stats_file()
        Test that if a child-process profiling-data file is corrupted,
        the profiling data therein are lost, but that doesn't affect the
        rest of the profiling session
    test_unwritable_purelib_path()
        Test if we can't write a .pth file, while we cannot set up
        profiling in non-forked children, it doesn't affect the rest of
        the profiling session

    Note: we omit tackling nested `kernprof` sessions (`P13.8`), because
    there isn't a(n easy) way to make it consistent.
@TTsangSC

TTsangSC commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Decided to just push so we have some progress that you/Fable can review while I work on the concurrent.futures stuff.

  • I cherry-picked the two commits working on the PR work items (9f2fbf7, 07dca5c). Will probably also merge the Cython stuff (2f36f08, c3b2915) but I want to take an even closer look at that code.
  • Status on the work items:
    • PR431-1:
      Merged with 9f2fbf7.
    • PR431-2:
      Merged with 07dca5c; revamped the protocol afterwards (d771ada) so that stats-reporting can reuse similar code (ac28767).1
    • PR431-3, PR431-12:
      • Updated .pth search path; rewrote the .pth file to be more tolerant (no error messages emitted if any import failed); implemented stale-pth cleanup (f6894f4).
      • Added new test tests/test_child_procs/test_abnormal_execution.py::test_unwritable_purelib_path() for the behavior when a .pth cannot be written (ee21829).
    • PR431-4:
      Fixed with c902c29.
    • PR431-5, PR431-6:
      Fixed with e7152a6.
    • PR431-7:
      TODO
    • PR431-8:
      Removed plugin/entry-point system; multiprocessing_patches._PATCHES now constructed with the new Registry.get_default() which hard-codes the list of patches (c28cc15).
    • PR431-9:
      Fixed with 14e9ee7.
    • PR431-10:
      Fixed with c3b72a9.
    • PR431-11:
      Instead of writing to the disk every task, we now send the stats back to the main process, which does the writing at process-finalization time.1 This exchanges disk-IO overhead with intra-process communication (pickling happens either way), so it should (hopefully) reduce the per-task overhead (ac28767).2
    • PR431-13:
      • Items 1, 5, 6:
        Fixed various lints/typos (14e9ee7).
      • Item 2:
        CacheLoggingEntry now carries an extra .level field; Cleanup.cleanup() failures now logged at the WARNING instead of DEBUG level.
      • Items 3, 7:
        Fixed with 14e9ee7.
      • Item 4:
        TODO; will code in a workaround when merging the Cython commits.
      • Item 8:
        WONTFIX at the moment, because there doesn't seem to be a consistent way to do it (nested kernprof sessions) – child processes rely on the ${LINE_PROFILER_PROFILE_CHILD_PROCESSES_CACHE_PID} envvar and the inner session would've overwritten it for the outer one, plus the inner and outer kernprof sessions could've been configured in inconsistent and irreconcilable ways.
    • PR431-T1, PR431-T4, PR431-T8, PR431-T9:
      Fixed with 4c4d2c5.
    • PR431-T2:
      Fixed with 14e9ee7.
    • PR431-T3:
      Limited scope of @cleanup_extra_pth_files; added doctest therefor (4c4d2c5).
    • PR431-T5:
      test_profiling_multiproc_script_{success,failure}() and now have one subtest running each multiprocessing start method in a true subprocess (1476d5a).
    • PR431-T6:
      • concurrent.futures:
        TODO; see previous discussions.
      • os.fork():
        Extendedtest_profiling_bare_python_{success,failure}() with os.fork() subtests (1476d5a).
      • Process.terminate():
        Added new test tests/test_child_procs/test_abnormal_execution.py::test_prematurely_terminated_process() (ee21829).
      • Corrupt .lprof:
        Added new test tests/test_child_procs/test_abnormal_execution.py::test_corrupted_child_stats_file() (ee21829).
      • os.environ:
        Updated _run_kernprof_main_in_process() to also check os.environ before and after calling kernprof.main() (1476d5a).
      • Nested sessions:
        WONTFIX; see PR431-13.
    • PR431-T7:
      WONTFIX; see previous discussions.
    • PR431-T10:
      Fixed with 4c4d2c5.
    • PR431-T11:
      Fixed with e2dd751; see also sys.path disparity between pytest --xdoc and pytest --doctest-modules Erotemic/xdoctest#207.

Welp, last pipeline failed, gotta debug.

Footnotes

  1. This also fixes the bug that if a worker has already died and been replaced at pool-termination time, end-of-process cleanup/bookkeeping doesn't happen. 2

  2. May be hard to verify with the current test suite though, because most tests run about the same number of workers and tasks.

@TTsangSC

TTsangSC commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Weird stuff, on Ubuntu the write of the .pth still went thru despite the write perms' being revoked. How is that even possible?

EDIT: I thought that maybe the spawned kernprof process inherited a writable handle to the venv's site-packages dir, but it is most probably not the case because subprocess.Popen(close_fds=...) already defaults to true.

@TTsangSC

TTsangSC commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Update on concurrent.futures tests:

  • Never mind the part about threads, I was just stupid and forgot that using Executor.map() implicitly causes tasks that haven't begun to be cancelled if earlier tasks errored out. Switching to an always-collect model like in process_test_module.py solved the issue.
  • Subinterpreters (Python 3.12+; as used by InterpreterPoolExecutor (Python 3.14+)) don't work at the moment: the immediate reason is that our Cython module hasn't declared support for that yet (Allow the user to declare subinterpreter support cython/cython#6513; Cython v3.1.0+), and as a result cannot be imported in subinterpreters, e.g. those created by concurrent.interpreters.create()). Will play around a bit with the module to check if we can get that to work robustly.

EDIT: okay I give up on trying to make the Cython module work in sub-interpreters. Will just update the _line_profiler_hooks.py so that it warns instead of raising an error if _line_profiler (and hence line_profiler) can't be imported.

TTsangSC added a commit that referenced this pull request Jul 20, 2026
line_profiler/autoprofile/profmod_extractor.py::_ImportTarget
    New dataclass as a more structured alternative to the dicts
    returned by the old

    .from_ast_nodes()
        Refactored from
        `ProfmodExtractor._ast_get_imports_from_tree()`, returning a
        list of the class from a sequence of nodes (so that it can be
        later refactored with ease for imports nested in other AST
        nodes (e.g. `FunctionDef.body`, `Try.orslse`))

line_profiler/autoprofile/profmod_extractor.py::ProfmodExtractor
    ._ast_get_imports_from_tree()
        Now a thin wrapper around `_ImportTarget.from_ast_nodes()`,
        returning a list of `_ImportTarget`s
    ._find_modnames_in_tree_imports()
        Each dict value in the return value now a `list[_ImportTarget]`
        instead of `list[str]`
    ._extract_all()
        - Refactored from `.extract_all()`, returning
          `dict[int, list[_ImportTarget]]` instead of
          `dict[int, list[str]]`
        - Now gracefully handling `from ... import *` statements (see
          item F4 in
          `fable-review-fullrepo-2026-07-05.md` of #5):
          instead of erroring out, we now issue a warning that it is not
          currently supported
    .extract_all()
        - Fixed erroneous return-value annotation in docstring
        - Now a thin wrapper around `._extract_all()`
    .run()
        Now wrapping around `._extract_all()` instead of
        `.extract_all()`, so as to emit more detailed warning messages
        with fully-qualified import-target names

tests/test_autoprofile.py::test_profmod_extractor_multitarget_behavior()
    - Updated the warning messages tested for
    - Added subtests where target-dropping by `ProfmodExtractor.run()`
      happens on multiple import statements (thus changing how the
      warning message is formatted)
    - Added subtests for warnings emitted when trying to profile
      `from ... import *` statements

TODO: fix star-imports in a future PR
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