bpo-40275: reduce importing module nums by lazy import in libregrtest #20207
Conversation
|
After this PR, the import module nums from 248 to 146 when I run |
|
@vstinner @serhiy-storchaka pls take a look if you have free time. |
There was a problem hiding this comment.
Hum. I'm not sure that this change reduces the number of imported modules when a test in run with "./python -m test test_os". For example, set_temp_dir() is called to run tests, and so tempfile is imported anyway. setup_tests() is also called in all cases before running a test. It's made on purpose.
Maybe we need a mechanism to load everything needed to run tests, and restore old sys.modules before loading tests. The risk is to load a module multiple times, but I don't think that it's an issue.
@serhiy-storchaka: What do you think?
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Oh, you are right.
Good idea. But I am not sure it will increse the complexity of unit test~ |
|
@vstinner Hi, victor.
|
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
|||
| @@ -1,18 +1,14 @@ | |||
| import asyncio | |||
There was a problem hiding this comment.
IMO these changes are useless, since saved_test_environment() is used by runtest(). I mean that all these imports are imported anyway.
If someone wants to reduce the number of imported modules, saved_test_environment() must be redesigned. Currently, it has no knowledge of what test is run, and so all checks are always run.
There was a problem hiding this comment.
OK, thanks, victor. I will check the detail of this function.
| @@ -2,7 +2,6 @@ | |||
| import re | |||
| import sys | |||
| import warnings | |||
| from inspect import isabstract | |||
There was a problem hiding this comment.
Rather than modifying refleak imports, it would be more generic to only import the refleak module when it's used.
Currently, clear_caches() is always called. Maybe we can only clear caches when -R 3:3 option is used.
There was a problem hiding this comment.
thanks, victor. I will try your suggestion.
There was a problem hiding this comment.
Rather than modifying refleak imports, it would be more generic to only import the refleak module when it's used.
Currently, clear_caches() is always called. Maybe we can only clear caches when -R 3:3 option is used.
about this idea, I found you and serihy discussed the clear_caches() in https://bugs.python.org/issue23839.
the point is that it's maybe raise some memoryError when calling clear_caches() after -R.
So I thought lazy import isabstract in dash_R() would be more safe(it would be called when we use -R 3:3 in tests.)
|
thanks victor for your suggestions. |
Make the the following imports lazy in lib.regrtest:
inspect, asyncio, logging, shutil, urllib, textwrap
Remove redundant import in lib.regrtest:
_multiprocessing
https://bugs.python.org/issue40275