-
Notifications
You must be signed in to change notification settings - Fork 131
FIX: kernprof -m
: presence of the executed module as sys.modules['__main__']
#339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CHANGELOG.rst Added entry kernprof.py::main() Now calling `runpy.run_module()` with `alter_sys = True` to maintain compatibility with code expecting the executed module to be found at `sys.modules['__main__']` line_profiler/autoprofile/autoprofile.py::run() Updated implementation to do the equivalent of `runpy.run_module(alter_sys=True)` tests/test_kernprof.py test_kernprof_m_parsing() - Updated implementation to expect the correct `sys.argv[0]` (real path to the module file) - Replaced `tempfile.mkdtemp()` with `tempfile.TemporaryDirectory` test_kernprof_m_sys_modules() New test for compatibility with tools (e.g. `@enum.global_enum`) expecting the executed module to be found at `sys.modules['__main__']`
We may want to give priority to this (over the other PRs) given that it's a fix instead of a new feature... |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
==========================================
+ Coverage 62.30% 63.63% +1.33%
==========================================
Files 13 13
Lines 1008 1034 +26
Branches 225 229 +4
==========================================
+ Hits 628 658 +30
+ Misses 316 315 -1
+ Partials 64 61 -3
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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.
Overall this looks great, and I had to go out of my way to find things to comment on. The only thing I think should be changed is the assert statement (unless I'm missing something and we really expect that to never happen, but it looks like a user-typo could cause that case to be hit).
line_profiler/autoprofile/autoprofile.py::run() - Replaced assertion with an explicitly raised `ModuleNotFoundError` - Streamlined logic to always call `importlib.util.find_spec()`, regardless of whether the module is found in `sys.modules` and already has a spec
Thanks for the review! Just updated the PR. |
Weird, the pipeline isn't running after I pushed the changes... did we recently change workflow-triggering policies? |
I haven't changed anything. Not sure why the CI isn't running. There isn't anything about a pending workflow that needs approval. It just isn't running and I don't see an option to force it to run. EDIT: I added a dummy commit, and it seems to run fine now. Not sure what was up. I'm unsatisfied with this hack, but at the same time I don't want to spend effort looking into it. Will keep an eye out if it happens again. |
Cheers! |
Motivation
kernprof -m
as it is now fails to address edge cases where the executed module is expected to be found atsys.modules['__main__']
. One such cases is for the@enum.global_enum
decorator, which searchessys.modules
for the module whither to insert the enum instances. This causes use-cases likekernprof -m calendar
to fail. Notably, a similar failure pattern is seen with even Pythonstdlib
modules.Changes
The key point of failure lies with how
kernprof
usesrunpy.run_module()
without the optionalalter_sys
argument. Since it defaults to false, the executed module is not inserted intosys.modules
, and thus the aforementioned use-case fails. Hence, this PR implements the following changes:CHANGELOG.rst
:Added entry
kernprof.py::main()
:Now calling
runpy.run_module()
withalter_sys = True
to maintain compatibility with code expecting the executed module to be found atsys.modules['__main__']
line_profiler/autoprofile/autoprofile.py::run()
:Updated implementation to do the equivalent of
runpy.run_module(alter_sys=True)
tests/test_kernprof.py
:test_kernprof_m_parsing()
:sys.argv[0]
(real path to the module file)tempfile.mkdtemp()
withtempfile.TemporaryDirectory
test_kernprof_m_sys_modules()
:New test for compatibility with tools (e.g.
@enum.global_enum
) expecting the executed module to be found atsys.modules['__main__']