gh-108927: Fix removing testing modules from sys.modules #108952

serhiy-storchaka · 2023-09-05T17:47:01Z

The code that does this properly never worked since Python 3.0.

And removing only the tested module breaks import machinery if it has submodules used in other tests.

Issue: test_unittest failure triggered by test_import + test_importlib #108927

It breaks import machinery if the test module has submodules used in other tests.

Yhg1s · 2023-09-05T18:22:06Z

Presumably the reload (the reason for the unload) was intentional. Do we know why that was added? Do those reasons still apply?

vstinner · 2023-09-05T18:35:52Z

Lib/test/libregrtest/runtest.py

@@ -434,10 +434,6 @@ def regrtest_runner(result, test_func, ns) -> None:
 def _load_run_test(result: TestResult, ns: Namespace) -> None:
    # Load the test function, run the test function.
    module_name = abs_module_name(result.test_name, ns.testdir)
-
-    # Remove the module from sys.module to reload it if it was already imported
-    sys.modules.pop(module_name, None)


Hum. I think that the problem is that only a single module is unload from sys.modules.

If we want to save/restore modules, IMO the whole sys.modules dictionary must be saved/restored. Not a single entry.

Would it make sense to save/restore the whole sys.modules dict? Would it fix #108927 ?

serhiy-storchaka · 2023-09-05T18:51:06Z

Tests crash with this change, and it means that there is other huge issue hidden until now.

If we want to save/restore modules, IMO the whole sys.modules dictionary must be saved/restored. Not a single entry.

It is already done in run_test() in main.py, so I do not understand why it did not have effect and why removing the module was needed here.

vstinner · 2023-09-06T02:05:01Z

Unloading the test module is done for 23 years (commit 5796d26) and so far, so I'm surprised that today it became a problem. Maybe it's not the right place to fix the issue.

commit 5796d26794eee634a4a06637d99d8d5c58da2bdb
Author: Guido van Rossum <[email protected]>
Date:   Fri Apr 21 21:35:06 2000 +0000

    Patch by Vladimir Marangozov to unload additionally imported modules
    after each test has been run.  This avoids excessive memory growth
    during the tests.

diff --git a/Lib/test/regrtest.py b/Lib/test/regrtest.py
index 6364d7179c..924890ab76 100755
--- a/Lib/test/regrtest.py
+++ b/Lib/test/regrtest.py
@@ -105,6 +105,7 @@ def main(tests=None, testdir=None):
     if single:
         tests = tests[:1]
     test_support.verbose = verbose      # Tell tests to be moderately quiet
+    save_modules = sys.modules.keys()
     for test in tests:
         if not quiet:
             print test
@@ -118,6 +119,10 @@ def main(tests=None, testdir=None):
                 print "test", test,
                 print "skipped -- an optional feature could not be imported"
             skipped.append(test)
+        # Unload the newly imported modules (best effort finalization)
+        for module in sys.modules.keys():
+            if module not in save_modules:
+                test_support.unload(module)
     if good and not quiet:
         if not bad and not skipped and len(good) > 1:
             print "All",

vstinner · 2023-09-06T02:08:21Z

It is already done in run_test() in main.py, so I do not understand why it did not have effect and why removing the module was needed here.

I would prefer to dig deeper before changing this code.

serhiy-storchaka · 2023-09-06T02:44:46Z

The crash is not related to this change. See #108976. If exclude test_pdb, all tests are passed successfully.

serhiy-storchaka · 2023-09-06T08:08:45Z

Unloading the test module is done for 23 years (commit 5796d26) and so far, so I'm surprised that today it became a problem.

Ah, that code does not work since 3.0.

It saves sys.modules.keys() which is a view in Python 3, and reflects the current content of sys.modules, so the code never finds any "newly imported" modules.

brettcannon · 2023-09-12T16:30:38Z

Lib/test/libregrtest/main.py

@@ -318,9 +318,9 @@ def run_tests_sequentially(self, runtests):
            result = self.run_test(test_name, runtests, tracer)

            # Unload the newly imported modules (best effort finalization)
-            for module in sys.modules.keys():
+            for module in list(sys.modules):


Suggested change

for module in list(sys.modules):

for module in sys.modules:

No it does not work, because the dict is modified during iteration.

Misc/NEWS.d/next/Tests/2023-09-05-20-46-35.gh-issue-108927.TpwWav.rst

…Wav.rst Co-authored-by: Brett Cannon <[email protected]>

vstinner · 2023-09-13T00:14:21Z

Lib/test/libregrtest/main.py

@@ -318,9 +318,9 @@ def run_tests_sequentially(self, runtests):
            result = self.run_test(test_name, runtests, tracer)

            # Unload the newly imported modules (best effort finalization)
-            for module in sys.modules.keys():
+            for module in list(sys.modules):


I don't understand this code. Why do we have to unload test modules?

If it's important to unload test modules, I would suggest to move this code inside single.py, to make a more consistent behavior for the different ways to run tests.

Do you ask why the code of libregrtest is so complicated? 🤷‍♂️

I think that it only makes sense in the single-process run. As well as removing the testing module before reloading in _load_run_test(). In the multi-process run it is only a waste of time. As well as significant part of save_env (except cleaning up external resources like filesystem).

vstinner · 2023-09-13T00:15:39Z

It saves sys.modules.keys() which is a view in Python 3, and reflects the current content of sys.modules, so the code never finds any "newly imported" modules.

Oh... I see :-) So if the code is dead for 20+ years, maybe remove it? I don't know what's the best option here.

I would expect that running a test would restore Python (sys.modules) in the exact same state.

vstinner

LGTM. Thanks for updated tests. I confirm that tests fail I revert Lib/test/libregrtest/ changes.

Lib/test/test_regrtest.py

Co-authored-by: Victor Stinner <[email protected]>

miss-islington-app · 2023-12-04T15:43:32Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

bedevere-app · 2023-12-04T15:43:41Z

GH-112711 is a backport of this pull request to the 3.12 branch.

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-app · 2023-12-04T15:43:45Z

GH-112712 is a backport of this pull request to the 3.11 branch.

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <[email protected]>

…08952) (ПР-112712) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <[email protected]>

…08952) (ПР-112711) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <[email protected]>

Yhg1s · 2023-12-05T16:25:39Z

It looks like the new test fails when running the tests from an installed Python installation (see https://buildbot.python.org/all/#/builders/350/builds/4974 and https://buildbot.python.org/all/#/builders/1154/builds/418), could someone take a look?

serhiy-storchaka · 2023-12-05T17:25:47Z

Thank you @Yhg1s. Please look at #112765.

hroncok · 2023-12-06T10:29:22Z

It looks like the new test fails when running the tests from an installed Python installation

FTR we are seeing the failures with the recently released 3.11.7.

vstinner · 2023-12-06T14:16:55Z

@hroncok:

FTR we are seeing the failures with the recently released 3.11.7.

I suppose that #112784 already fixed the issue in the 3.11 branch. Sadly, this fix is not part of 3.11.7 release.

Buildbots were too slow to report the failure on the 3.11 branch before the release? It seems like we have "Installed" buildbot builders on the 3.11 branch.

hroncok · 2023-12-06T14:18:33Z

Yes. What's done is done, but may I suggest a release not be made with commits before the buildbots have time to recognize a failure?

vstinner · 2023-12-06T14:23:57Z

3.11 commit 8dbda1c: Mon Dec 4 17:16:03 2023 +0100
Git tag v3.11.7: Mon Dec 4 17:56:31 2023 +0000

Ah right, there were less than 1 hour between the commit and the tag.

@hroncok:

Yes. What's done is done, but may I suggest a release not be made with commits before the buildbots have time to recognize a failure?

Usually, the branch is not locked around bugfix 3.x.y releases.

cc @pablogsal 3.11 release manager.

pablogsal · 2023-12-06T14:43:01Z

The release script blocks if there are failing build bots so this was releaseable because all buildbots were green at the time (or the API reported green).

Is very difficult to time this correctly because even if we block a branch we do this hours before the release so we don't know when all builsbots have finished.

We can discuss blocking a day in advance to prevent this from happening or some other mechanism.

I will discuss this with other RMs to see what can be done.

vstinner · 2023-12-06T20:38:03Z

In my experience, it's rare to discover bugs so soon just after a release. I'm not sure that it's worth it to deeply change the release process.

Maybe the release process should run tests on installed Python, rather than in the source code directory.

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests.

serhiy-storchaka added type-bug An unexpected behavior, bug, or error tests Tests in the Lib/test dir labels Sep 5, 2023

serhiy-storchaka requested a review from vstinner September 5, 2023 17:47

bedevere-bot added the awaiting core review label Sep 5, 2023

bedevere-bot mentioned this pull request Sep 5, 2023

test_unittest failure triggered by test_import + test_importlib #108927

Closed

pythongh-108927: Do not remove tested modules from sys.modules

339f260

It breaks import machinery if the test module has submodules used in other tests.

serhiy-storchaka force-pushed the regretest-no-remove-test-module branch from 4f7617c to 339f260 Compare September 5, 2023 17:47

Yhg1s mentioned this pull request Sep 5, 2023

gh-108927: Fix test_import + test_importlib + test_unittest problem #108929

Merged

vstinner reviewed Sep 5, 2023

View reviewed changes

serhiy-storchaka added 2 commits September 6, 2023 11:00

Update NEWS entry.

e52be9b

Fix unloading the newly imported modules after testing.

68c014e

serhiy-storchaka changed the title ~~gh-108927: Do not remove tested modules from sys.modules~~ gh-108927: Fix removing testing modules from sys.modules Sep 6, 2023

serhiy-storchaka added 2 commits September 12, 2023 10:41

Merge branch 'main' into regretest-no-remove-test-module

c1ad70c

Add tests.

3f25ab4

serhiy-storchaka requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners September 12, 2023 08:52

brettcannon approved these changes Sep 12, 2023

View reviewed changes

Update Misc/NEWS.d/next/Tests/2023-09-05-20-46-35.gh-issue-108927.Tpw…

7673248

…Wav.rst Co-authored-by: Brett Cannon <[email protected]>

vstinner reviewed Sep 13, 2023

View reviewed changes

serhiy-storchaka added 2 commits September 13, 2023 10:55

Move save/unload modules down in single.py.

5df4c8b

Merge branch 'main' into regretest-no-remove-test-module

eb7766b

vstinner approved these changes Dec 4, 2023

View reviewed changes

Lib/test/test_regrtest.py Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting core review labels Dec 4, 2023

Update Lib/test/test_regrtest.py

b3ea8fa

Co-authored-by: Victor Stinner <[email protected]>

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Dec 4, 2023

serhiy-storchaka enabled auto-merge (squash) December 4, 2023 15:11

serhiy-storchaka merged commit e08b70f into python:main Dec 4, 2023
31 checks passed

bedevere-app bot removed the awaiting merge label Dec 4, 2023

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Dec 4, 2023

bedevere-app bot removed the needs backport to 3.11 only security fixes label Dec 4, 2023

serhiy-storchaka deleted the regretest-no-remove-test-module branch December 5, 2023 17:12

pablogsal mentioned this pull request Jan 9, 2024

test_unload_tests fails in Python 3.11 when executed from an installed directory #113862

Closed

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-108927: Fix removing testing modules from sys.modules (pytho…

a096f48

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-108927: Fix removing testing modules from sys.modules (pytho…

6db4be2

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-108927: Fix removing testing modules from sys.modules #108952

gh-108927: Fix removing testing modules from sys.modules #108952

serhiy-storchaka commented Sep 5, 2023 •

edited

Loading

Yhg1s commented Sep 5, 2023

vstinner Sep 5, 2023

serhiy-storchaka commented Sep 5, 2023

vstinner commented Sep 6, 2023

vstinner commented Sep 6, 2023

serhiy-storchaka commented Sep 6, 2023

serhiy-storchaka commented Sep 6, 2023

brettcannon Sep 12, 2023

serhiy-storchaka Sep 12, 2023

vstinner Sep 13, 2023

serhiy-storchaka Sep 13, 2023

vstinner commented Sep 13, 2023 •

edited

Loading

vstinner left a comment

miss-islington-app bot commented Dec 4, 2023

bedevere-app bot commented Dec 4, 2023

bedevere-app bot commented Dec 4, 2023

Yhg1s commented Dec 5, 2023

serhiy-storchaka commented Dec 5, 2023

hroncok commented Dec 6, 2023

vstinner commented Dec 6, 2023

hroncok commented Dec 6, 2023

vstinner commented Dec 6, 2023

pablogsal commented Dec 6, 2023

vstinner commented Dec 6, 2023

gh-108927: Fix removing testing modules from sys.modules #108952

gh-108927: Fix removing testing modules from sys.modules #108952

Conversation

serhiy-storchaka commented Sep 5, 2023 • edited Loading

Yhg1s commented Sep 5, 2023

vstinner Sep 5, 2023

Choose a reason for hiding this comment

serhiy-storchaka commented Sep 5, 2023

vstinner commented Sep 6, 2023

vstinner commented Sep 6, 2023

serhiy-storchaka commented Sep 6, 2023

serhiy-storchaka commented Sep 6, 2023

brettcannon Sep 12, 2023

Choose a reason for hiding this comment

serhiy-storchaka Sep 12, 2023

Choose a reason for hiding this comment

vstinner Sep 13, 2023

Choose a reason for hiding this comment

serhiy-storchaka Sep 13, 2023

Choose a reason for hiding this comment

vstinner commented Sep 13, 2023 • edited Loading

vstinner left a comment

Choose a reason for hiding this comment

miss-islington-app bot commented Dec 4, 2023

bedevere-app bot commented Dec 4, 2023

bedevere-app bot commented Dec 4, 2023

Yhg1s commented Dec 5, 2023

serhiy-storchaka commented Dec 5, 2023

hroncok commented Dec 6, 2023

vstinner commented Dec 6, 2023

hroncok commented Dec 6, 2023

vstinner commented Dec 6, 2023

pablogsal commented Dec 6, 2023

vstinner commented Dec 6, 2023

serhiy-storchaka commented Sep 5, 2023 •

edited

Loading

vstinner commented Sep 13, 2023 •

edited

Loading