Replace inspect usage with common CPython stdlib pattern
#327
Conversation
…panded version of a frame inspection pattern used in the standard library. References: - https://github.com/python/cpython/blob/e2476398ee9911b6b0b80e3ca182647805fde81f/Lib/enum.py#L871-L879 - https://github.com/python/cpython/blob/e2476398ee9911b6b0b80e3ca182647805fde81f/Lib/doctest.py#L228-L234 - https://github.com/python/cpython/blob/e2476398ee9911b6b0b80e3ca182647805fde81f/Lib/dataclasses.py#L1625-L1632 - https://github.com/python/cpython/blob/e2476398ee9911b6b0b80e3ca182647805fde81f/Lib/typing.py#L1840-L1849 - https://github.com/taleinat/python-stdlib-sentinels/blob/410273e40bdbd1b58a63cebefdf8abcce6c620e3/sentinels/sentinels.py#L95-L124
|
This can be slimmed down (as evidenced by the stdlib's version), but I figured I'd start with the most robust version. |
Sachaa-Thanasius
changed the title
inspect usage with common CPython stdlib pattern
There was a problem hiding this comment.
Thanks for this PR. It's a great start and definitely a good concrete example of a possible approach. I have a number of concerns, some of which I'm not confident we can resolve easily... and others which we shouldn't bother considering until we've settled on a high level approach.
Let's start by answering my questions and exploring the possibility of supplying a private _inspect.stack() that essentially vendors a lightweight version of inspect.stack() suitable for the purposes here.
| except AttributeError: # For platforms without sys._getframemodulename. | ||
| global _get_caller_module_name | ||
|
|
||
| def _get_caller_module_name(depth: int = 1, default: str = "__main__") -> str: |
There was a problem hiding this comment.
I don't care for how __main__ gets snuck in here. It wasn't required before. Why have a fallback now (instead of retaining parity)?
| Walk the stack and find the frame of the first caller not in this module. | ||
| """ | ||
| # An expanded version of a CPython stdlib pattern to avoid using the expensive inspect. | ||
| def _get_caller_module_name(depth: int = 1, default: str = "__main__") -> str: |
There was a problem hiding this comment.
I don't love the repetitive nature of this function. For example, the literal "__main__" is used multiple times, and the default depth=1 is mentioned three times. And depth+1 has to be implemented twice. I'd like to see it refactored so that these common concerns are implemented in exactly one place.
| # PYUPDATE: 3.15 - Update depth & explanation for package_to_anchor()'s removal. | ||
| # Expected parent stack frames for depth=4: | ||
| # 0) resolve()'s singledispatch dispatch | ||
| # 1) resolve()'s singledispatch wrapper | ||
| # 2) files() | ||
| # 3) package_to_anchor() | ||
| # 4) <caller>() |
There was a problem hiding this comment.
I don't love this comment. It's nice to know, but it distracts from the function's purpose and represents tech debt. Usually, I like to abstract tech debt into separate modules that provide the compatible and future uses. If we end up sticking with the static depth for detecting the caller, I'd like to see the depth itself encapsulated in a _caller_depth() function, whose docstring explains why the number 4 is there, when it might need to change, and maybe a static copy of the current call stack (although I think I'd rather omit that instead ensure there are tests capturing the expectation).
Are there tests that fail if the depth doesn't match the implementation?
| try: | ||
| frame = sys.exc_info()[2].tb_frame # type: ignore[union-attr] # Guarded. | ||
| for _ in range(depth + 1): | ||
| frame = frame.f_back |
There was a problem hiding this comment.
I don't love that the current implementation goes from 2 levels of indentation to 6. I'll take it on myself to refactor the implementation to be more of a composition of smaller functions.
| for _ in range(depth + 1): | ||
| frame = frame.f_back | ||
| return frame | ||
| except Exception: |
There was a problem hiding this comment.
I don't love the bare Exception here. What is its purpose?
| try: | ||
| raise TypeError | ||
| except TypeError: |
There was a problem hiding this comment.
It took me a while to understand the purpose of this code. It's there to cause the interpreter to generate a stack trace. There should probably be a comment to this effect.
| # also exclude 'wrapper' due to singledispatch in the call stack | ||
| callers = itertools.filterfalse(is_wrapper, not_this_file) | ||
| return next(callers).frame | ||
| return _get_frame() |
There was a problem hiding this comment.
While the previous implementation was just a few lines, this new implementation has a lot of hacks and internal details. We probably should move it all to its own _inspect module.
| # 2) files() | ||
| # 3) package_to_anchor() | ||
| # 4) <caller>() | ||
| return resolve(_get_caller_module_name(depth=4)) |
There was a problem hiding this comment.
I'm quite uneasy hard-coding literals (like depth=4). I understand it represents the expected call stack, but now it needs to be kept in sync with the implementation, entangling those concerns. The current implementation avoided entangling those concerns by tracking a more abstract concept (the first caller in the stack that's not this module). Is there a reason not to instead simply implement a replacement for inspect.stack() that avoids inspect?
| # This attempts to support Python implementations that either don't have sys._getframe | ||
| # (e.g. Jython) or don't support sys._getframe(x) where x >= 1 (e.g. IronPython). | ||
| # We avoid inspect.stack because of how expensive inspect is to import. |
There was a problem hiding this comment.
I'm not sure it's worth supporting Jython or IronPython. Maybe best would be to simply rely on inspect in those cases.
Part of #326.
This is the biggest source of import time and hopefully the least controversial to replace; based on local testing, replacing this improves it by ~30% on supported CPython versions and PyPy3.10.
This replaces the expensive
inspectimport andinspect.stack()usage with an expanded version of a frame inspection pattern used in the CPython standard library.It's a bit more code, but it's a fairly reasonable amount considering it does the following:
References: