FIX: Don't revert the entirety of sys.modules after running kernprof.main#437
Conversation
tests/test_kernprof.py
chdir_temp
New helper context manager for creating a new tempdir and
`cd`-ing into it, factored out from common pattern in tests
test_*()
Updated to use the above
test_kernprof_m_import_resolution()
test_kernprof_eager_preimport_bad_module()
Replaced hard-coded ':' in the `${PYTHONPATH}` value with
`os.pathsep`
test_ppe_pickling()
New test which uses `concurrent.futures.ProcessPoolExecutor` in
code run by `kernprof.main()`; the case where PPE hasn't been
imported prior to `kernprof.main()` being called twice currently
fails
kernprof.py
_restore
Migrated to `line_profiler.line_profiler_utils.restore`
main()
Instead of reverting the entire module namespace of
`line_profiler.diagnostics`, now only reverting
`line_profiler.diagnostics.log` (which is replaced in
`_parse_arguments()`)
_main_profile()
Instead of reverting the entire `sys.modules`, now only
reverting `sys.modules['__main__']`
line_profiler/autoprofile/autoprofile.py::run()
Replaced internal context manager with a `restore` instance
line_profiler/line_profiler_utils.py::restore
- Migrated from `kernprof.py::_restore`
- Added type annotations
- Added optional argument `keys` (resp. `attrs`) to `.mapping()`
(resp. `.instance_dict()`) so that users can selectively revert
certain key-value pairs (resp. attributes)
- Now reentrant
- Note: the attributes `.setter`, `.getter`, and `.obj` are removed
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #437 +/- ##
==========================================
+ Coverage 82.40% 82.76% +0.36%
==========================================
Files 20 20
Lines 2256 2298 +42
Branches 359 361 +2
==========================================
+ Hits 1859 1902 +43
Misses 300 300
+ Partials 97 96 -1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Erotemic
left a comment
There was a problem hiding this comment.
This all LGTM. Minor comments. You can choose what to do with them. Say the word and I'll merge.
|
|
||
|
|
||
| def main() -> None: | ||
| with ProcessPoolExecutor(mp_context=get_context('spawn')) as ex: |
There was a problem hiding this comment.
| with ProcessPoolExecutor(mp_context=get_context('spawn')) as ex: | |
| with ProcessPoolExecutor(max_workers=2, mp_context=get_context('spawn')) as ex: |
Not sure if we want to use all CPUs on a test. On one hand it could expose more edge cases on the other, setting it is more stable.
| short_string_path, | ||
| ) | ||
| from line_profiler.line_profiler_utils import ( | ||
| restore as _restore, # Compatibility |
There was a problem hiding this comment.
Compatibility for a private name isn't required. It doesn't bother me though. This is mostly a nitpick.
There was a problem hiding this comment.
Fair enough, since we're technically already changing restore's API (it no longer has .getter, .setter, and .old – but then again those members aren't used anywhere by the code outside of the class itself). Might as well just import under the new name.
| python_path = str(temp_dpath) | ||
| if 'PYTHONPATH' in os.environ: | ||
| python_path += ':' + os.environ['PYTHONPATH'] | ||
| python_path += os.pathsep + os.environ['PYTHONPATH'] |
kernprof.py
No longer importing `line_profiler.line_profiler_utils.restore`
under the name `_restore` since it isn't fully compatible to the old
`_restore` anyway
tests/test_kernprof.py::test_ppe_pickling()
Now explicitly passing `max_workers` to PPE to avoid spinning up too
many processes
|
Thanks for the quick review, should be alright now (other than the |
|
Yeah I ignored the ty error for that reason, let's land #434 next |
Closes #436.
(EDIT: typos; missing summaries for
kernprof.y::main()andline_profiler/autoprofile/autoprofile.py::run())Code changes
line_profiler/line_profiler_utils.py::restore:kernprof.py::_restore, so that other parts of the codebase can use it for cleanup..mapping()(resp..instance_dict()) now takes an extra argumentkeys: Collection[K](resp.attrs: Collection[str]) to limit the scope of reversion..getter,.setter, and.old. (But we don't really use it anywhere other than as anonymous decorators and context managers anyway, so nothing changed.)kernprof.py::main():On exit:
sys.modules, we now only revertsys.modules['__main__']or the lack thereof.line_profiler._diagnosticsnamespace, we now only revertline_profiler._diagnostics.logwhich is overridden.line_profiler/autoprofile/autoprofile.py::run():Instead of freezing the entire
sys.moduleswhen running the supplied code, we now only revertsys.modules['__main__']or the lack thereof.Test changes
line_profiler/line_profiler_utils.py::restore.sequence()'s doctest extended to demonstrate/check for reentrant behavior.tests/test_kernprof.py:cd-ing therein into a helper context manager (chdir_temp).test_kernprof_m_import_resolution()andtest_kernprof_eager_preimport_bad_module()now useos.pathsepinstead of a hard-coded':'to join${PYTHONPATH}components.test_ppe_picklingformalizes the failing case in Revertingsys.modulescauses pickling issues withconcurrent.futures.ProcessPoolWorker#436, checking that repeated invocation ofkernprof.main()runningconcurrent.futurescode doesn't fail due to consistency issues withsys.modules.