FIX: handling multi-target (from-)import statements#434
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #434 +/- ##
==========================================
+ Coverage 82.76% 84.59% +1.82%
==========================================
Files 20 20
Lines 2298 2395 +97
Branches 361 376 +15
==========================================
+ Hits 1902 2026 +124
+ Misses 300 271 -29
- Partials 96 98 +2
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Fair enough. Should I also begin working on |
Erotemic
left a comment
Erotemic
left a comment
There was a problem hiding this comment.
Overall the patch looks right, there are few minor polishing issues to take care of, and then we can merge.
| if True, when auto-profiling whole script, profile all imports aswell. | ||
|
|
||
| ast_transformer_class_handler (Type): | ||
| ast_transformer_class_handler (type[AstProfileTransformer]): |
There was a problem hiding this comment.
I'm unsure of the modern utility of these google style docstring blocks. When I started using them back in Python 2.7 they were indispensable. Even in the early 3.x days I had strong arguments that these were better than the weak state of type annotations. But now type annotations in Python are a lot better, and these are mostly redundant. My only concern about dropping them would be the impact they have on readthedocs. I'm not sure if the type annotations in the signature are properly merged with the parsed documentation here. Nothing needs to be done about it here, but it's on my mind.
|
|
||
| def run( | ||
| self, *, assume_single_target_imports: bool = True, | ||
| ) -> dict[int, str] | dict[int, list[str]]: |
There was a problem hiding this comment.
This new return type is introduced in this PR? We might simplify the typing if we just deprecate this function and introduce a new one with the new types.
There was a problem hiding this comment.
... yeah on second thought that's definitely the way to go, IDK what I was on (probably too much chips and cola).
Code coverage should still be preserved with minimal additional fixes to the test suite given that we're already directly constructing a ProfmodExtractor in test_profmod_extractor_multitarget_behavior().
We may want to think a bit more about what the new method looks like. Maybe
def extract_all(self) -> dict[int, list[str]]:
...and then .run() could just become
def run(self) -> dict[int, str]:
warnings.warn(DeprecationWarning, ...)
result: dict[int, str] = {}
dropped: set[str] = set()
for loc, names in self.extract_all().items():
*remainder, last = names
dropped.update(remainder)
result[i] = last
if dropped:
warnings.warn(...)
return resultThere was a problem hiding this comment.
extract_all makes sense. Nothing is really "running". I think that's a better method name.
| r"2 .* target.* dropped .* \['ham', 'spam'\]")]) | ||
| def test_profmod_extractor_multitarget_behavior( | ||
| prof_mod: list[str], | ||
| expected: dict[str, int] | dict[str, list[int]], |
There was a problem hiding this comment.
Key and value type ares reversed here. Are we not running type checks on the tests? We probably should be. I'll have to check that.
There was a problem hiding this comment.
Good catch(es).
- ... yeah no, there's currently no type checks for the test suite on CI. Probably intentional because most of the older tests aren't type-annotated and would've been skipped (at least by
mypy, probably notty) anyway. - When writing new tests I always type-annotate them and run them through both
mypyandty. On local, at least. - But this one's on me and is really unfortunate –
expectedis only used in one place in the test (equality check with the return value), and as a result anything goes.
Will fix.
There was a problem hiding this comment.
Update: ty is very noisy when applied to the (older) test modules.
- The number 1 reason is of course that it peeks into untyped functions with (AFAIK) no option to turn that off, unlike
mypy's--[no]-check-untyped-defs. - The number 2 reason is that a lot of tests use
ubelt.util_cmd.cmd(...) -> CmdOutput, and since unlikesubprocess.CompletedProcessubelt.util_cmd.CmdOutputisn't parametrized (seetypeshed/stdlib/subprocess.pyi),tyhas to assume the worst aboutCmdOutput.stdoutand.stderr(that they arestr | bytes | None). But even with parametrization, introducing all the@overloads upstream inubeltto make this work is probably gonna be a huge PITA.
| depr_warning_pattern = 'assume_single_target_imports' | ||
| targets_warning_pattern = 'profiling target.* dropped.* multi-target' | ||
| checks: list[Callable[[Sequence[WarningMessage]], None]] = [] | ||
| with contextlib.ExitStack() as stack: |
There was a problem hiding this comment.
Your temp dir helper from the other PR probably cleans this up a bit? It can stay as is for this one.
There was a problem hiding this comment.
Here we don't (nor do we need to) chdir, just a tempdir to hold the singular tempfile. Semantically tempfile.NamedTemporaryFile would've been a better fit, but I've had bad experience of that misbehaving esp. on Windows. And we'd still need the ExitStack either way because we want to catch and analyze the warnings.
As for tests/test_kernprof.py::chdir_temp, I wonder if it makes better sense to (1) patch it up into a fixture and plonk it into a conftest.py, or (2) make a new test_<sometime>_utils.py and have tests import it from there? Probably the latter, fixture-izing it has the disadvantage of more coarse-grained lifetime control (the tempdir itself and the cd would be alive for the entirely of the test).
There was a problem hiding this comment.
experience of that misbehaving esp. on Windows
I've had the same bad experience.
I've probably said this before, but I really don't like fixtures. They feel too magic for me. I want to trace any statement I express to either the base language or a line that imports it from somewhere else. I don't want to have to be aware of magic context when I run code - or have to explain it to someone else when I'm teaching them.
That being said, I've acquiesced on this. Fixtures are ubiquitous and for some reason other good developers seem to like them. But in this case, I do prefer the test_<sometime>_utils.py approach.
| if assume_single_target_imports is None: # Default | ||
| result: dict[int, str] | dict[int, list[str]] = extractor.run() | ||
| else: | ||
| result = extractor.run( # `mypy` needs a bit of help here |
There was a problem hiding this comment.
Mypy is failing here because I think you are missing an explicit overload on bool. The reason is very ugly.
Consider:
from typing import Literal, overload
@overload
def func(flag: Literal[True]) -> str: ...
@overload
def func(flag: Literal[False]) -> int: ...
def func(flag: bool) -> str | int:
return "yes" if flag else 0
literal_result = func(True) # OK: str
flag: bool = bool(input())
dynamic_result = func(flag) # Type-checker error
@overload
def func2(flag: Literal[True]) -> str: ...
@overload
def func2(flag: Literal[False]) -> int: ...
# Yeah, this is unintuitive and ugly.
# The function itself doesn't count
@overload
def func2(flag: bool) -> str | int
def func2(flag: bool) -> str | int:
return "yes" if flag else 0
flag: bool = bool(input())
dynamic_result = func2(flag) # no issue
Reason is documented here.
The non-@overload-decorated definition, meanwhile, will be used at runtime but should be ignored by a type checker
I'm just learning about this. It feels very ugly. The reason seems to be the runtime function needs a broader scope that encompass everything, but if that broad scope is also valid, it would be nice if typing gave a way to mark it as such.
|
|
||
| warnings = stack.enter_context(catch_warnings(record=True)) | ||
| if assume_single_target_imports in (True, None): | ||
| checks.append(partial( |
There was a problem hiding this comment.
This is just a test so I'm not going to block on this, but for whatever reason building up lists of partial functions and executing them feels obfuscating to me. I generally prefer patterns where I build up some concrete set of data and then make the execution dependent on the data, rather than difficult to inspect function pointers.
There was a problem hiding this comment.
If I'm understanding correctly, you mean that you'd prefer something like ...?
checks: list[tuple[bool, str, type[Warning]]] = []
if assume_single_target_imports in (True, None):
checks.append((True, depr_warning_pattern, DeprecationWarning))
...
for warning_expected, pattern, WarningType in checks:
regex = re.compile(pattern)
matches: list[WarningMessages] = [
msg for msg in msgs
if issubclass(msg.category, WarningType)
if regex.search(msg.message)
]
assert bool(matches) == warning_expected, ... # TODO: format the messageI guess it is clearer this way and avoids an additional stack frame. Will do.
There was a problem hiding this comment.
Yeah, roughly. My default is almost always to think in terms of List[Dict] row organized data frames, so the names are clear at pack and unpack times. But it is unnecessary overhead. A list[tuple] is perfectly clear here, and I'm starting to get used to the pattern now that I've been diving into rust.
The main issue I like to avoid is having to reason about what exact function is being called via the symbols used in the code. This is much more explicit and easier to reason about. (for me at least)
| ) | ||
| issue_warning(msg, DeprecationWarning, stacklevel=2) | ||
| conflated_result: dict[int, str] = {} | ||
| dropped_names: set[str] = set() |
There was a problem hiding this comment.
Looks like this would fail for something where a name is imported and then redefined. Not sure if we want to handle this case or not. We probably should if it isn't too hard. But we don't want this static analysis to end up becoming a full execution runtime either.
import foo, bar
import baz as fooThere was a problem hiding this comment.
Adding a dropped_names.discard(last) should be easy enough, will do.
What just came to my mind though is that currently any import statement not directly in Module.body is skipped. Beside imports nested inside functions and classes (whether we want to deal with which is debatable1), this also means that some other common patterns are also ignored:
import json # This is extracted
from sys import version_info
from typing import TYPE_CHECKING
if version_info[:2] >= (3, 11) or TYPE_CHECKING:
import tomllib # ... but this isn't
else:
import tomli as tomlib # ... and neither is this
try:
from os import fork # ... nor this
except ImportError:
_HAS_FORK = False
else:
_HAS_FORK = True
def main():
# ... and finally not this; XXX: but do we *want* to cater to this?
import textwrap
...But I guess that can be another issue and PR.
Footnotes
-
I vaguely remember this being mentioned in some discussion somewhere in the repo. Don't remember where though, nor whether it was you and I or someone else in the discussion. ↩
There was a problem hiding this comment.
I vaguely recall that too, and I think the decision was to punt on it at the time. I don't remember exactly what the conversation was but looking at this code and thinking about the problem I'm 99% sure I didn't want to have to resolve arbitrary expressions in a static check. And I also think that nested imports are something that should be documented as explicitly out of scope for autoprofile. Working on static code means we are forced to make certain assumptions to get the speed and safety of static analysis.
| f'^Function: {prefix}{func}', raw_output, re.MULTILINE | ||
| ) | ||
| assert bool(in_output) == (func in profiled_funcs) | ||
|
|
There was a problem hiding this comment.
Might also include an executable variant of the tests. This was suggested by GPT 5.6
| def test_multitarget_import_transformation_executes() -> None: | |
| """ | |
| Test the runtime behavior of the transformed AST, including: | |
| - multiple targets in one import statement; | |
| - aliases; | |
| - preservation of profiling-call order; | |
| - passing the actual imported objects to the profiler. | |
| """ | |
| from xml.etree.ElementTree import Element, XMLParser | |
| input_module = ub.codeblock( | |
| """ | |
| import os, sys as system | |
| from xml.etree.ElementTree import Element, XMLParser as Parser | |
| """ | |
| ) | |
| with tempfile.TemporaryDirectory() as tmp: | |
| fpath = ub.Path(tmp) / 'script.py' | |
| fpath.write_text(input_module) | |
| module_ast = AstTreeProfiler( | |
| str(fpath), | |
| [ | |
| 'os', | |
| 'sys', | |
| 'xml.etree.ElementTree.Element', | |
| 'xml.etree.ElementTree.XMLParser', | |
| ], | |
| False, | |
| ).profile() | |
| profiled_objects = [] | |
| class RecordingProfiler: | |
| def add_imported_function_or_module(self, obj) -> None: | |
| profiled_objects.append(obj) | |
| namespace = { | |
| 'profile': RecordingProfiler(), | |
| } | |
| code = compile(module_ast, str(fpath), 'exec') | |
| exec(code, namespace) | |
| assert profiled_objects == [ | |
| os, | |
| sys, | |
| Element, | |
| XMLParser, | |
| ] | |
| # Also verify that the aliases created by the original imports resolve | |
| # to the same objects that were passed to the profiler. | |
| assert namespace['system'] is sys | |
| assert namespace['Element'] is Element | |
| assert namespace['Parser'] is XMLParser |
There was a problem hiding this comment.
Actually I just realized that this is largely covered by test_multitarget_import_resolution() which directly checks the ast.unparse()-ed source code, but of course it doesn't hurt to have a more "in-code" test that directly works with objects and namespaces. I've also taken the liberty to update this new test with a target which isn't selected by prof_mod.
| (['foo', 'foobar.ham'], {0: 'foo', 2: 'ham'}, True, None), | ||
| # Also test not passing `assume_single_target_imports` (defaults to | ||
| # true) | ||
| (['foo', 'foobar.ham'], {0: 'foo', 2: 'ham'}, True, None), |
There was a problem hiding this comment.
| (['foo', 'foobar.ham'], {0: 'foo', 2: 'ham'}, True, None), | |
| (['foo', 'foobar.ham'], {0: 'foo', 2: 'ham'}, None, None), |
Should the True be None? Otherwise this is just the same as the previous case.
|
|
||
| - Warn against dropped profiling targets (if any) | ||
|
|
||
| ``assume_single_target_imports=True`` |
There was a problem hiding this comment.
| ``assume_single_target_imports=True`` | |
| ``assume_single_target_imports=False`` |
Typo?
Either way is fine. You're doing all the hard work. At this point, I'm just acting as the skeptical independent quality gate for your excellent contributions. |
line_profiler/autoprofile/
ast_tree_profiler.py::AstTreeProfiler.profile()
Updated implementation to account for multiple import targets
in the same (from-)import statement
profmod_extractor.py
- Removed unused typing imports
- Updated implementation and return type of
`ProfmodExtractor.run()` to account for multiple import
targets in the same (from-)import statement
XXX:
should we add a switch for `ProfmodExtractor.run()` so that it
behaves as before by default?
CHANGELOG.rst
Added entry
tests/test_autoprofile.py::test_multitarget_import_resolution()
New parametrized test testing that issue pyutils#433 has been fixed
line_profiler/autoprofile/ast_tree_profiler.py::AstTreeProfiler
__init__()
Updated type hints for `ast_transformer_class_handler` and
`profmod_extractor_class_handler` to be more specific
profile()
Updated call to `ProfmodExtractor.run()` to include
`assume_single_target_imports=False` (see below)
line_profiler/autoprofile/profmod_extractor.py::ProfmodExtractor.run()
Updated call signature:
- New parameter `assume_single_target_imports` defauling to true,
restoring the legacy behavior
- If `assume_single_target_imports` is true, a `DeprecationWarning`
is issued; and if it results in an valid profiling target being
dropped, a `UserWarning` is issued
- If false, the new behavior (returning a `dict[int, list[str]]`) is
used
tests/test_autoprofile.py::test_profmod_extractor_multitarget_behavior()
New parametrized test to cover the aforementioned warning behavior,
ensure that backward compatibility is retained while users are
warned against the shortcomings
|
Thanks for the review and the inputs. Will work on them after dinner. IDK if this is the right place, but I just want to say that I'm glad and grateful that you're here to QA (and maintain the repo in general) – in this day and age code is cheap, and QA is only getting more and more important.1 LLMs have their uses, as was well-demonstrated by your TTsangSC#5;2 but I'd imagine that you did a lot of prompt engineering,3 planning, harnesses, and precautions behind the scene, for things to have gone as well as they did. And when more and more people just use the tools to churn out code and issues en masse, unfortunately without such degree of thought and care... yeah. Footnotes
|
Yes it is. If you're feeling it, and then its the right place. I often feel like the "this is not the right place" is more often made to silence people and dismiss unpleasant realities than it is to guide productive conversation.
I strongly encourage you not to sleep on LLMs. I'm biased as I'm an AI researcher. But they are the real deal. At least the frontier ones are. I'm holding out that Qwen4 can start to approach opus or gpt 5.5 level. Not a huge fan of the power concentration and sociopolitical structure around them, but from an objective "I want to do good work and make the world a better place" standpoint, their value is unquestionable IMHO. July of 2025 was when I think they really crossed the threshold towards true usefulness.
I'm actually a little offended none of my repos are getting hit, perhaps they aren't big enough targets 🤷. My view on it is that they have lowered the bar required to contribute or get your foot in the door. I view that as a good thing. I'm not a big fan of gate-keeping. I think they go a long way towards empowering people to do incredible new things. But it's not all good, and there are major growing pains right now. I hope my optimism is well-placed. But I think we need to start expecting more from people. There needs to be a higher bar on critical thought, inquisitiveness, and integrity. Not knowing about something existent hasn't been an excuse since Wikipedia was a thing, and now its even less of an excuse.
Probably more than I think. I'm a trained scientist, I consider myself a critical thinker, and I place an enormous value on truth and honesty. Sadly those terms have been co-opted these days. But I don't know any better words. I don't have the full trace for my prompt history to get the fable review, but it looks like I started off by working with GPT 5.5 just pointing it at the URLs for these PRs and then I had it build a prompt for Fable. Note: GPT 5.6 is a big leap over 5.5, but it wasn't out at the time, and 5.5 can be iffy at points. In any case the prompt it constructed was something like: Details
And this is probably from me throwing around the words "elegant" a lot. I always ask it: "Is this code the elegant way to accomplish the goal"? Or something to that effect. I try to use the Socratic method with it, so I force a debate to try and poke holes in ideas that are presented. My actual prompt I type aren't nearly that long, but I'll often be discussing plans with one LLM and then when we seem to come to a consensus I tell it to write a prompt for another agent. I think I only added one prompt after fable did its thing, and that was with an opus model: Details
Really it's the same precautions I use with people. I ask questions where things don't make sense and I try to cut to the technical core of the problem.
I find myself wondering the same thing. We still need to solve the power problem and scale these things down, but the future is far more uncertain than its ever been for me. I really thought vision was going to be the problem that neural networks would solve - and they are - but the NLP folks surprised me. I really didn't expect the Turing test to go down in my lifetime, but here we are. |
line_profiler/autoprofile/ast_tree_profiler.py
::AstTreeProfiler.profile()
Now calling `ProfmodExtractor.extract_all()`
line_profiler/autoprofile/profmod_extractor.py
ProfmodExtractor.run()
- Reverted the `assume_single_target_imports` argument
(071222a); the whole method (1) always use the pre-PR type
signature and (2) is just deprecated
- Added a check that a dropped name isn't shadowed by a later
import so as to reduce false alarms
ProfmodExtractor.extract_all()
New method behaving like `assume_single_target_imports=False`
before this commit, returning `dict[int, list[str]]` instead of
`.run()`'s `dict[int, str]`
_issue_warning()
New private function extracted from inside
`ProfmodExtractor.run()`; fixed warning attribution by adding a
`stacklevel` to the `warnings.warn()` call
tests/test_autoprofile.py
test_profmod_extractor_multitarget_behavior()
- Replaced parameter `assume_single_target_imports` with
`method`, which chooses whether to call
`ProfmodExtractor.run()` or `.extract_all()`
- Fixed type annotation of parameter `expected`
- Added subtests for when a later imports shadows a name in an
older one
- Refactored implementation to be more explicit and readable
with the warning-message checks
test_multitarget_import_transformation_executes()
New test similar to `test_multitarget_import_resolution()` using
`AstTreeProfiler()` to check the transformed code, except that
instead of inspecting the code text, we `exec()` the code and
verify its effects on a namespace
|
|
||
| Returns: | ||
| (Dict[int,str]): tree_imports_to_profile_dict | ||
| tree_imports_to_profile_dict (dict[int, str] | dict[int, list[str]]); |
There was a problem hiding this comment.
| tree_imports_to_profile_dict (dict[int, str] | dict[int, list[str]]); | |
| tree_imports_to_profile_dict (dict[int, list[str]]); |
I need to figure out exactly how sphinx readthedocs interacts with these return strings and the type annotations. We shouldn't have to double specify them. The only thing I'm reserved about is that I've grown so used to seeing the type right next to the help string, which I find valuable. Uggg, if only Python had a nice way to attach help docs to types. Annotated exists, but I think it's ugly.
There was a problem hiding this comment.
Poked around a bit with a local Sphinx build:
Note:
prof_moddoesn't have its type in parenthesis next to it, unlike the other params. That is because I deleted its Google-style annotation before the build just ot see what would happen- The param types are picked up by Sphinx and are separately formatted (with
<em>instead of<strong>), but don't result in resolved links to the docs of the type objects. - All the params still have their type annotations parsed from the code as seen in the blue banner above. The return-type annotation for
.extract_all()is likewise shown in the gray method. The type hints in the banner are also resolved and clickable. - Meanwhile the erroneous Google-style annotation for the return value of
.extract_all()(next totree_imports_to_profile_dict) doesn't seem to be special-cased by Sphinx (as it did with the parameters), and is just formatted with<dd>text like most of the rest of theReturns:segment.
So I guess it does suffice to just keep type annotations in-code and not duplicate them in-doc: the in-code annotations are more functional, and maintaining a separate set of annotations leaves us open to gaffes like this where we forget to update the docstring. But this comes at the cost of not having the type next to the per-param doc as you've said. Still, if the concern is only with the webdocs instead of the docstrings themselves, tox-dev/sphinx-autodoc-typehints purportedly does the job of auto-interpolating the annotations back.
As for typing.Annotated... yeah that's ugly. One thing that I've used it for though is to provide extra info without actually modifying the param types, esp. for user-facing functions:
class AlNumMeta(type):
def __instancecheck__(cls, obj: object) -> bool:
return isinstance(obj, str) and obj.isalnum()
class AlNumString(str, metaclass=AlNumMeta):
...
def func(alnum_str: Annotated[str, AlNumString]) -> None:
# If we annotated `alnum_str` with `AlNumString` directly, the runtime behavior doesn't
# change, but static typing would go ballistic on a user doing e.g. `func('12345')` instead
# of `func(AlNumString('12345'))`
assert isinstance(alnum_str, AlNumString)
...But even then one can argue that it's probably more productive to either just put the extra info in the docs or use a value object if one needs assurance that the arg .isalnum().
| _issue_warning(msg, DeprecationWarning, stacklevel=2) | ||
| result: dict[int, str] = {} | ||
| dropped_names: set[str] = set() | ||
| for i, names in self.extract_all().items(): |
There was a problem hiding this comment.
Looks like there is still an issue here:
Details
import sys
import types
# Create three distinct importable module objects.
module_names = [
'lp_shadow_demo_foo',
'lp_shadow_demo_bar',
'lp_shadow_demo_baz',
]
modules = {
name: types.ModuleType(name)
for name in module_names
}
sys.modules.update(modules)
class RecordingProfiler:
def __init__(self):
self.seen = []
def add_imported_function_or_module(self, obj):
self.seen.append(obj)
try:
# This is the grouped result that extract_all() would produce:
#
# import lp_shadow_demo_foo as x, lp_shadow_demo_bar
# -> ['x', 'lp_shadow_demo_bar']
#
# import lp_shadow_demo_baz as x
# -> ['x']
grouped = {
0: ['x', 'lp_shadow_demo_bar'],
1: ['x'],
}
def current_accounting(grouped):
"""
Equivalent to the current PR implementation.
"""
result = {}
dropped_names = set()
for tree_index, names in grouped.items():
*remainder, last = names
dropped_names.update(remainder)
# Current attempted shadow handling
dropped_names.discard(last)
result[tree_index] = last
dropped_names -= set(result.values())
return result, dropped_names
def correct_accounting(grouped):
"""
Preserve dropped bindings per import statement.
"""
result = {}
dropped = {}
for tree_index, names in grouped.items():
*remainder, last = names
result[tree_index] = last
if remainder:
dropped[tree_index] = remainder
return result, dropped
current_result, current_dropped = current_accounting(grouped)
correct_result, correct_dropped = correct_accounting(grouped)
# Execute the program that a consumer of the lossy legacy result can
# construct. Only the last selected name from each import statement is
# available.
source = """
import lp_shadow_demo_foo as x, lp_shadow_demo_bar
profile.add_imported_function_or_module(lp_shadow_demo_bar)
import lp_shadow_demo_baz as x
profile.add_imported_function_or_module(x)
"""
profile = RecordingProfiler()
exec(source, {'profile': profile})
requested_objects = [
modules['lp_shadow_demo_foo'],
modules['lp_shadow_demo_bar'],
modules['lp_shadow_demo_baz'],
]
missing_objects = [
obj
for obj in requested_objects
if not any(obj is seen for seen in profile.seen)
]
print('Objects requested:')
print([obj.__name__ for obj in requested_objects])
print('\nObjects actually profiled:')
print([obj.__name__ for obj in profile.seen])
print('\nObjects actually dropped:')
print([obj.__name__ for obj in missing_objects])
print('\nCurrent accounting says dropped:')
print(current_dropped)
print('\nCorrect statement-local accounting says dropped:')
print(correct_dropped)
finally:
for name in module_names:
sys.modules.pop(name, None)So... thinking about it. Do we even need this function? extract_all seems to handle it correctly. I don't think this is part of the public API. Can we just hard-deprecate run and raise a RuntimeError if someone calls it? I almost want to just delete it, but we should probably be nice in the rare case someone is using it, and tell them what to call instead.
There was a problem hiding this comment.
IDK... on the one hand, I do think a lot of line_profiler.autoprofile could've been private, and basically has been since a while to the end-user – and yeah, since ProfmodExtractor.run() doesn't work correctly nor can it, it makes a lot of sense to do away with the method. On the other, ProfmodExtractor and its methods (incl. run()) have been public API since as long as auto-profiling has been a thing, and having them in the docs without any additional qualifications only serves to reinforce that. But at the end of the day my take is that maybe we shouldn't entirely drop the method before 6.0.1
Not sure if I follow as to the issue though. I thought the failure that you originally mentioned when shadowing occurs was that since the name is shadowed (and thus not used further down the code), it doesn't matter if it's correctly resolved and/or profiled, and thus reporting it in the warning would've been a false positive.
But looking at the newer example, it seems that your issue is with how we're only (not) reporting that x is dropped, without actually pointing the user towards what x actually is (lp_shadow_demo_foo or lp_shadow_demo_baz)? So in this case, your concern is that it should be reported that... ?
UserWarning: 1 import target(s) dropped in multi-target import statements:
- Line 1: `x` (= `lp_shadow_demo_foo`)
Of course this can be done, but we'll need extra metadata that .extract_all() elided. Maybe we can update ._find_modnames_in_tree_imports()2 to return dict[int, list[_ModuleDict]]3 instead of dict[int, list[str]], and have both .extract_all() and .run() be thin wrappers around that which strip the unnecessary metadata?
Footnotes
-
Just so that we're on the same page, is the current plan to have this and FEAT: extend profiling to child processes #431 in 5.1 or 6.0? Line Profiler 6.0 Roadmap #374 is there, but IDK where on the timeline we are. ↩
-
Again this tempers with the return types on methods, but at least it's a private one this time. ↩
-
BTW I think this module would hugely benefit from having a
_ModuleDicttyped dict/data-class type, with which we can annotate e.g. the return value of._ast_get_imports_from_tree(). Maybe I'll go ahead and do just that. ↩
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
|
Did a bit of refactoring of CI seems to be stuck in limbo though – it's been 15 full minutes and none of the jobs has started. Any idea why? EDIT ah yeah Actions are borked. EDIT 2 fixed |
line_profiler/autoprofile/ast_tree_profiler.py::AstTreeProfiler
._profile_ast_tree()
Updated to accommodate the new return type of
`ProfmodExtractor.extract_all()` (see below)
._descend()
New helper method for descending into nested objects given a
chain of indices and attribute names
line_profiler/autoprofile/profmod_extractor.py
ImportTarget
- Renamed from `_ImportTarget`
- Added caution to delineate the public API
- Renamed method `.from_ast_nodes()` -> `._from_ast_nodes()`
- Removed property `._star_import_source`
- Property `.resolved_name` is now `None` for star-imports
ProfmodExtractor
._ast_get_imports_from_tree()
Changed key type in returned dict to
`tuple[str | int, ...]]` in preparation of allowing nested
imports (e.g. conditional imports) to be processed
.extract_all()
- Supersedes `._extract_all()`
- Changed return type to
`dict[tuple[str | int, ...], list[ImportTarget]]` to allow
for future extensiblity (e.g. handling nested imports and
star-imports)
- Updated implementation
- Now clearly documenting that only top-level imports are
currently handled
.run()
- Updated implementation
- Now clearly documenting that only top-level imports are
handled
tests/test_autoprofile.py::test_profmod_extractor_multitarget_behavior()
Updated implementation so that the parametrization remains simple:
we only supply the `Module.body` indices in the dict keys, and the
lists of `ImportTarget.resolved_name`s in the dict values
|
Just realized that since we're going through the trouble of introducing a new method which is technically public API to replace a deprecated one (again, the public methods of
As such, I've refactored |
I feel like if it isn't a top level In the meantime, I think just having the RuntimeError is fine, and we shouldn't overthink it. |
2 participants
Closes #433.
Changes
line_profiler.autoprofile.profmod_extractor.ProfmodExtractor:.run()is now deprecated with aDeprecationWarning, and in addition aUserWarningwhen using it results in profiling targets being dropped from the output..extract_all()method returns insteaddict[tuple[str | int, ...], list[AstTreeProfiler]]so that multi-target imports can be handled, and that we retain enough flexibility to handle other cases like star-imports and conditional imports via future extensions, without having to change the API like in this PR.from ... import *statements (see item F4 infable-review-fullrepo-2026-07-05.mdin Fable review of profile child process TTsangSC/line_profiler#5): instead of resulting in syntactically wrongprofile.add_imported_function_or_module(*)statements, star-imports are now dropped from the output of.run()and.extract_all()with a warning.line_profiler.autoprofile.profmod_extractor.ImportTargetclass for hauling info around.line_profiler.autoprofile.ast_tree_profiler.AstTreeProfilerhas been correspondingly updated to insertprof.add_imported_function_or_module()statements for each eligible target in multi-target imports.tyconfig line topyproject.tomlto guard against v0.0.52's making redundant casts an error.tests/test_autoprofile.pyto verify the fixes:test_multitarget_import_resolution()andtest_multitarget_import_transformation_execution()check thatAstTreeProfiler.profile()now correctly transforms ASTs with multi-target import statements.test_profmod_extractor_multitarget_behavior()checks thatProfmodExtractor.run()'s return type and warning issuance are as aformentioned.