FIX: handling multi-target (from-)import statements #434
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,12 +3,14 @@ | |||||
| import ast | ||||||
| import os | ||||||
| import sys | ||||||
| from typing import Literal, cast | ||||||
| from warnings import warn | ||||||
| from .util_static import ( | ||||||
| modname_to_modpath, | ||||||
| modpath_to_modname, | ||||||
| package_modpaths, | ||||||
| ) | ||||||
| from .. import _diagnostics as diagnostics | ||||||
|
|
||||||
|
|
||||||
| class ProfmodExtractor: | ||||||
|
|
@@ -30,7 +32,7 @@ def __init__( | |||||
| script_file (str): | ||||||
| path to script being profiled. | ||||||
|
|
||||||
| prof_mod (list[str]): | ||||||
| list of imports to profile in script. | ||||||
| passing the path to script will profile the whole script. | ||||||
| the objects can be specified using its dotted path or full path (if applicable). | ||||||
|
|
@@ -77,13 +79,13 @@ def _get_modnames_to_profile_from_prof_mod( | |||||
| script_file (str): | ||||||
| path to script being profiled. | ||||||
|
|
||||||
| prof_mod (list[str]): | ||||||
| list of imports to profile in script. | ||||||
| passing the path to script will profile the whole script. | ||||||
| the objects can be specified using its dotted path or full path (if applicable). | ||||||
|
|
||||||
| Returns: | ||||||
| modnames_to_profile (list[str]): | ||||||
| list of dotted paths to profile. | ||||||
| """ | ||||||
| script_directory = os.path.realpath(os.path.dirname(script_file)) | ||||||
|
|
@@ -105,7 +107,7 @@ def _get_modnames_to_profile_from_prof_mod( | |||||
| so we check if the item is path and whether that path exists, else skip the item. | ||||||
| """ | ||||||
| modpath = modname_to_modpath( | ||||||
| mod, sys_path=cast('list[str | os.PathLike]', new_sys_path) | ||||||
| ) | ||||||
| if modpath is None: | ||||||
| """if cannot convert to modpath, check if already path and if invalid""" | ||||||
|
|
@@ -148,7 +150,7 @@ def _ast_get_imports_from_tree( | |||||
| abstract syntax tree to fetch imports from. | ||||||
|
|
||||||
| Returns: | ||||||
| module_dict_list (list[Dict[str, str | int]]): | ||||||
| list of dicts of all imports in the tree, containing: | ||||||
| name (str): | ||||||
| the real name of the import. e.g. foo from "import foo as bar" | ||||||
|
|
@@ -193,7 +195,7 @@ def _ast_get_imports_from_tree( | |||||
| def _find_modnames_in_tree_imports( | ||||||
| modnames_to_profile: list[str], | ||||||
| module_dict_list: list[dict[str, str | int | None]], | ||||||
| ) -> dict[int, list[str]]: | ||||||
| """Map modnames to imports from an abstract sytax tree. | ||||||
|
|
||||||
| Find imports in modue_dict_list, created from an abstract syntax tree, that match | ||||||
|
|
@@ -205,21 +207,22 @@ def _find_modnames_in_tree_imports( | |||||
| The import's alias is stored in the output dict. | ||||||
|
|
||||||
| Args: | ||||||
| modnames_to_profile (list[str]): | ||||||
| list of dotted paths to profile. | ||||||
|
|
||||||
| module_dict_list (list[Dict[str, str | int]]): | ||||||
| list of dicts of all imports in the tree. | ||||||
|
|
||||||
| Returns: | ||||||
| modnames_found_in_tree (dict[int, list[str]]): | ||||||
| dict of imports found | ||||||
| key (int): | ||||||
| index of the (from-)import statement in AST | ||||||
| value (list[str]): | ||||||
| list of aliases (or names if no alias used) to | ||||||
| import | ||||||
| """ | ||||||
| modnames_found_in_tree: dict[int, list[str]] = {} | ||||||
| modname_added_list = [] | ||||||
| for i, module_dict in enumerate(module_dict_list): | ||||||
| modname = module_dict['name'] | ||||||
|
|
@@ -240,30 +243,97 @@ def _find_modnames_in_tree_imports( | |||||
| tree_index = module_dict['tree_index'] | ||||||
| if not isinstance(tree_index, int): | ||||||
| raise TypeError('should have gotten an int') | ||||||
| modnames_found_in_tree.setdefault(tree_index, []).append(name) | ||||||
|
Member
There was a problem hiding this comment. This is a nitpick, because this is a very standard pattern, but I've always found it difficult to parse. I would prefer the dumb check and initialize, and then append, or using a collections.defaultdict. Feel free to ignore this, I'm not going to enforce this preference. |
||||||
| return modnames_found_in_tree | ||||||
|
|
||||||
| def extract_all(self) -> dict[int, list[str]]: | ||||||
| """Map prof_mod to imports in an abstract syntax tree. | ||||||
|
|
||||||
| Takes the paths and dotted paths in prof_mod and finds their respective imports in an | ||||||
| abstract syntax tree, returning their aliases and the index they appear in the AST. | ||||||
|
|
||||||
| Returns: | ||||||
| tree_imports_to_profile_dict (dict[int, str] | dict[int, list[str]]); | ||||||
|
Member
There was a problem hiding this comment.
Suggested change
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.
Collaborator
Author
There was a problem hiding this comment. Poked around a bit with a local Sphinx build: Note:
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, As for 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 |
||||||
| dict of imports to profile | ||||||
| key (int): | ||||||
| index of import in AST | ||||||
| value (str | list[str]): | ||||||
| list of aliases (or names if no alias used) to | ||||||
| import | ||||||
| """ | ||||||
| modnames_to_profile = self._get_modnames_to_profile_from_prof_mod( | ||||||
| self._script_file, self._prof_mod | ||||||
| ) | ||||||
|
|
||||||
| module_dict_list = self._ast_get_imports_from_tree(self._tree) | ||||||
|
|
||||||
| return self._find_modnames_in_tree_imports( | ||||||
| modnames_to_profile, module_dict_list, | ||||||
| ) | ||||||
|
|
||||||
| def run(self) -> dict[int, str]: | ||||||
| """ | ||||||
| Deprecated, legacy method kept for backward compatibility. | ||||||
|
|
||||||
| Returns: | ||||||
| tree_imports_to_profile_dict (dict[int, str]) | ||||||
| dict of imports to profile | ||||||
| key (int): | ||||||
| index of import in AST | ||||||
| value (str): | ||||||
| alias (or name if no alias used) of the LAST | ||||||
| target to import in the corresponding | ||||||
| :py:class:`ast.Import` or | ||||||
| :py:class:`ast.ImportFrom` statement | ||||||
|
|
||||||
| Notes: | ||||||
| - New code should use the :py:meth:`.extract_all` method, | ||||||
| which handles multi-target import statements (see #434). | ||||||
|
|
||||||
| - Calling this method issues a | ||||||
| :py:class:`DeprecationWarning`. | ||||||
|
|
||||||
| - For multi-target import statements, this only preserves | ||||||
| the last target. If this results in import targets being | ||||||
| dropped, a :py:class:`UserWarning` is issued. | ||||||
| """ | ||||||
| msg = ( | ||||||
| '`ProfmodExtractor.run()` is now deprecated, because it cannot ' | ||||||
| 'correctly resolve multi-target import statements; ' | ||||||
| 'use `ProfmodExtractor.extract_all()` instead.' | ||||||
| ) | ||||||
| _issue_warning(msg, DeprecationWarning, stacklevel=2) | ||||||
| result: dict[int, str] = {} | ||||||
| dropped_names: set[str] = set() | ||||||
|
Member
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 foo
Collaborator
Author
There was a problem hiding this comment. Adding a What just came to my mind though is that currently any import statement not directly in 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
Member
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. |
||||||
| for i, names in self.extract_all().items(): | ||||||
|
Member
There was a problem hiding this comment. Looks like there is still an issue here: Detailsimport 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
Collaborator
Author
There was a problem hiding this comment. IDK... on the one hand, I do think a lot of 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 Of course this can be done, but we'll need extra metadata that Footnotes
|
||||||
| *remainder, last = names | ||||||
| dropped_names.update(remainder) | ||||||
| # In case a later import shadows a dropped name from an | ||||||
| # earlier import | ||||||
| dropped_names.discard(last) | ||||||
| result[i] = last | ||||||
| dropped_names -= set(result.values()) | ||||||
| if dropped_names: | ||||||
| msg = ( | ||||||
| '{}: {} would-be profiling target(s) dropped because the ' | ||||||
| 'import statement(s) are multi-target: {!r}' | ||||||
| ).format( | ||||||
| self._script_file, len(dropped_names), sorted(dropped_names), | ||||||
| ) | ||||||
| _issue_warning(msg, stacklevel=2) | ||||||
| return result | ||||||
|
|
||||||
|
|
||||||
| def _issue_warning( | ||||||
| msg: str, | ||||||
| category: type[Warning] | None = None, | ||||||
| stacklevel: int = 1, | ||||||
| *args, | ||||||
| **kwargs, | ||||||
| ) -> None: | ||||||
| if category is None: | ||||||
| log_msg = msg | ||||||
| else: | ||||||
| log_msg = f'{category.__name__}: {msg}' | ||||||
| diagnostics.log.warning(log_msg) | ||||||
| warn(msg, category, stacklevel + 1, *args, **kwargs) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.