GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_pickle/test_pickletools fail when running tests sequentially. · Issue #103247 · python/cpython

test_pickle/test_pickletools fail when running tests sequentially. #103247

New issue
Copy link
New issue
Copy link
Closed
Closed
test_pickle/test_pickletools fail when running tests sequentially.#103247
Copy link
Labels
interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)release-blockertopic-importlibtopic-subinterpreterstype-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

Description

@Yhg1s
Yhg1s
opened on Apr 4, 2023
Issue body actions

When running the test suite sequentially (as is done as part of the release process), an interaction between test_imp, test_importlib and subinterpreters is causing test_pickle and test_pickletools to fail. The easiest reproducer::

% ./python -m test test_imp test_importlib test_pickletools
0:00:00 load avg: 0.22 Run tests sequentially
0:00:00 load avg: 0.22 [1/3] test_imp
0:00:00 load avg: 0.22 [2/3] test_importlib
0:00:03 load avg: 0.22 [3/3] test_pickletools
test test_pickletools failed -- Traceback (most recent call last):
  File "Lib/test/pickletester.py", line 1989, in test_builtin_types
    s = self.dumps(t, proto)
        ^^^^^^^^^^^^^^^^^^^^
  File "Lib/test/test_pickletools.py", line 11, in dumps
    return pickletools.optimize(pickle.dumps(arg, proto, **kwargs))
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_pickle.PicklingError: Can't pickle <class 'importlib._bootstrap.BuiltinImporter'>: it's not the same object as importlib._bootstrap.BuiltinImporter

test_pickletools failed (1 error)

== Tests result: FAILURE ==

2 tests OK.

1 test failed:
    test_pickletools

Total duration: 4.2 sec
Tests result: FAILURE

The failure happens in both test_pickle and test_pickletools (test_pickletools's traceback is more obvious), after in the same process running both test_imp and either test_import or test_importlib. Disabling test_imp.ImportTests.test_create_builtin_subinterp() makes the tests pass. (This was happening before #102982 as well as after it, so I don't think it is involved here.)

I expect this is an unintentional side-effect of the test, but this was not happening in the 3.12.0a6 release, and as far as I can tell the relevant tests haven't changed since before a6. I'm delaying the 3.12.0a7 release until it's clear this isn't a fundamental problem with subinterpreters.

Linked PRs

  • GH-103247: bypass the import cache on the _require_loader helper #104186
  • gh-103247: add tearDown to test_loader and clear module cache #104226
  • [3.11] gh-103247: clear the module cache in a test in test_importlib/extensions/test_loader.py (GH-104226) #104345

Activity

Next
  1. Yhg1s
    added
    type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error
    release-blocker
    on Apr 4, 2023
  2. Yhg1s
    assigned
    ericsnowcurrently
    on Apr 4, 2023
  3. ezio-melotti
    added this to Release and Deferred blockers 🚫on Apr 4, 2023
  4. github-project-automation
    moved this to Todo in Release and Deferred blockers 🚫on Apr 4, 2023
  5. Yhg1s commented on Apr 4, 2023

    @Yhg1s
    Yhg1s
    on Apr 4, 2023
    MemberAuthor
    More actions

    The problem is actually reproducible all the way back to 3.12.0a4, even though it was never caught in the release testing (I don't know why, yet). The problem was exposed by #100142, but that merely adds testing of picklability of builtins, which all things considered is not an unreasonable test.

  6. Yhg1s commented on Apr 4, 2023

    @Yhg1s
    Yhg1s
    on Apr 4, 2023
    MemberAuthor
    More actions

    As it appears this is merely uncovering a long standing issue with a hole in subinterpreter isolation and importlib (the same test combination fails in 3.10 and 3.11), I'm unblocking a7 for now, but leaving this as a release blocker for beta 1.

    Reacted by Erlend E. Aasland
  7. ericsnowcurrently
    removed their assignment
    on Apr 4, 2023
  8. ericsnowcurrently
    added
    topic-subinterpreters
    on Apr 4, 2023
  9. ezio-melotti
    added this to Subinterpreterson Apr 4, 2023
  10. ericsnowcurrently
    added
    interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)
    on Apr 4, 2023
  11. warsaw
    added
    topic-importlib
    on Apr 4, 2023
  12. erlend-aasland commented on Apr 5, 2023

    @erlend-aasland
    erlend-aasland
    on Apr 5, 2023
    Contributor
    More actions

    I did some bisecting in 3.10 and 3.11, and it landed like this:

    • In 3.10, first bad commit: 37dbbb2 ([3.10] gh-99578: Fix refleak in _imp.create_builtin() (GH-99642) #99644)
    • In 3.11, first bad commit: 9dda902 ([3.11] gh-99578: Fix refleak in _imp.create_builtin() (GH-99642) #99643)

    Strangely, I do not get the same result in main; the "parent" commit (cb2ef8b, gh-99642) that those backports were created from, reproduces the failure, as expected, but cb2ef8b2acbb231c207207d3375b2f8b0077a6ee~ (1cae31d) is not a "good" commit; the failure also occurs there.
    The first bad commit I get in main is:

    • 1f369ad (gh-98354: Add unicode check for 'name' attribute in _imp_create_builtin #98412)

    The linked issues for those PRs contain some interesting history and debugging:

    • gc_decref: Assertion "gc_get_refs(g) > 0" failed #99578
    • create_builtin() in _imp module trigger segfault if taking a builtin object as input #98354

    (It would be nice if someone else could bisect on their machine and verify my findings.)

  13. sunmy2019 commented on Apr 5, 2023

    @sunmy2019
    sunmy2019
    on Apr 5, 2023
    Member
    More actions

    even though it was never caught in the release testing (I don't know why, yet).

    Is it because the release testing in running in 4 processes?

  14. Yhg1s commented on Apr 5, 2023

    @Yhg1s
    Yhg1s
    on Apr 5, 2023
    MemberAuthor
    More actions

    No, the release testing is done sequentially, and that hasn't changed in 2 years: https://github.com/python/release-tools/blame/master/run_release.py#L486

  15. 4 remaining items

  16. JelleZijlstra commented on May 4, 2023

    @JelleZijlstra
    JelleZijlstra
    on May 4, 2023
    Member
    More actions

    test_imp is no more, so the original reproduction steps no longer work. However, I see this failure on current main (no pickle tests necessary):

    % ./python.exe -m test test_importlib test_import
    Raised RLIMIT_NOFILE: 256 -> 1024
    0:00:00 load avg: 1.91 Run tests sequentially
    0:00:00 load avg: 1.91 [1/2] test_importlib
    0:00:10 load avg: 2.29 [2/2] test_import
    test test_import failed -- Traceback (most recent call last):
      File "/Users/jelle/py/cpython/Lib/test/test_import/__init__.py", line 1815, in test_multi_init_extension_compat
        require_extension(module)
      File "/Users/jelle/py/cpython/Lib/test/test_import/__init__.py", line 90, in require_extension
        _require_loader(module, ExtensionFileLoader, skip)
      File "/Users/jelle/py/cpython/Lib/test/test_import/__init__.py", line 76, in _require_loader
        actual = MODULE_KINDS[actual]
                 ~~~~~~~~~~~~^^^^^^^^
    KeyError: <class 'importlib._bootstrap_external.ExtensionFileLoader'>
    
    test_import failed (1 error)
    
    == Tests result: FAILURE ==
    
    1 test OK.
    
    1 test failed:
        test_import
    
    Total duration: 13.5 sec
    Tests result: FAILURE
    
  17. FFY00
    added a commit that references this issue on May 5, 2023

    pythonGH-103247: bypass the import cache on the _require_loader helper

    8be54d3
  18. sunmy2019 commented on May 5, 2023

    @sunmy2019
    sunmy2019
    on May 5, 2023
    Member
    More actions

    test_imp is no more, so the original reproduction steps no longer work. However, I see this failure on current main (no pickle tests necessary):

    I looked into it. The failure (changing the env) comes from

    cpython/Lib/test/test_importlib/extension/test_loader.py

    Lines 263 to 273 in e5b8b19

    def test_try_registration(self):
    # Assert that the PyState_{Find,Add,Remove}Module C API doesn't work.
    module = self.load_module()
    with self.subTest('PyState_FindModule'):
    self.assertEqual(module.call_state_registration_func(0), None)
    with self.subTest('PyState_AddModule'):
    with self.assertRaises(SystemError):
    module.call_state_registration_func(1)
    with self.subTest('PyState_RemoveModule'):
    with self.assertRaises(SystemError):
    module.call_state_registration_func(2)

    Commenting on it solves this issue.

    It seems not to follow the with util.uncache(self.name) idiom in adjacent tests. (I did not check the code logic, just did A/B test)

  19. FFY00 commented on May 6, 2023

    @FFY00
    FFY00
    on May 6, 2023
    Member
    More actions

    Good catch! Do you want to open a PR with the fix?

    I think it'd make sense to add a teardown method which unloads self.name instead of relying on util.uncache to be used in all tests.

  20. sunmy2019 commented on May 6, 2023

    @sunmy2019
    sunmy2019
    on May 6, 2023
    Member
    More actions

    Good catch! Do you want to open a PR with the fix?

    #104226

  21. brettcannon
    added 2 commits that reference this issue on May 10, 2023

    Merge branch 'main' into pythongh-103247

    43ef567

    gh-103247: clear the module cache in a test in test_importlib/extensi…

    22f3425
  22. miss-islington
    added a commit that references this issue on May 10, 2023

    pythongh-103247: clear the module cache in a test in test_importlib/e…

    06a0974
  23. JelleZijlstra
    added a commit that references this issue on May 10, 2023

    [3.11] gh-103247: clear the module cache in a test in test_importlib/…

    1cbf844
  24. carljm
    added a commit that references this issue on May 10, 2023

    Merge branch 'main' into fixloadsuperspec

    5976df9
  25. brettcannon
    closed this as completedon May 12, 2023
  26. github-project-automation
    moved this from Todo to Done in Release and Deferred blockers 🚫on May 12, 2023
  27. github-project-automation
    moved this to Done in Subinterpreterson May 12, 2023
  28. FFY00
    added 2 commits that reference this issue on Aug 24, 2023

    Merge branch 'main' into pythongh-103247-_require_loader

    e18626c

    GH-103247: bypass the import cache on the _require_loader helper

    feb9a49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)release-blockertopic-importlibtopic-subinterpreterstype-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

    Projects

    • Release and Deferred blockers 🚫
      Status
      Done
      Show more project fields
    • Subinterpreters
      Status
      Done
      Show more project fields

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

    You are visitor 01402503 — GeezerVison does not store the pages you look up.
    Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.