test: Ignore error message give from python because of PYTHON_GIL #33795

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:skipIfGILTestError changing 1 files +10 −1
  1. kevkevinpal commented at 11:13 PM on November 5, 2025: contributor

    Description

    I was able to reproduce this issue #33582

    Currently, if a user is running Python version 3.14.0t they will get this error message and the test suite will halt.

    stderr:
<frozen importlib._bootstrap>:491: RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp', which has not declared that it can run safely without the GIL. To override this behavior and keep the GIL disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.

    Instead of halting, we should ignore and suppress the warning message because, regardless of PYTHON_GIL being set, our tests should pass.

    In this change, we are avoiding setting PYTHON_GIL=1 in case it is deprecated in the future, so that newer tests would avoid having thread safety bugs. It makes sense not to have our test suite depend on PYTHON_GIL=1

    Before and after applying this patch

    Before: Warning message when using Python version 3.14.0t and PYTHON_GIL unset on master branch

    ./build/test/functional/test_runner.py interface_ipc.py
Temporary test directory at /var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/test_runner_₿_🏃_20251009_094704
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py failed, Duration: 7 s

stdout:
2025-10-09T08:47:05.067786Z TestFramework (INFO): PRNG seed is: 8793929945784019901
2025-10-09T08:47:05.069073Z TestFramework (INFO): Initializing test directory /var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/test_runner_₿_🏃_20251009_094704/interface_ipc_0
2025-10-09T08:47:05.880408Z TestFramework (INFO): Running echo test
2025-10-09T08:47:05.883429Z TestFramework (INFO): Running mining test
2025-10-09T08:47:12.059233Z TestFramework (INFO): Stopping nodes
2025-10-09T08:47:12.173668Z TestFramework (INFO): Cleaning up /var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/test_runner_₿_🏃_20251009_094704/interface_ipc_0 on exit
2025-10-09T08:47:12.173842Z TestFramework (INFO): Tests successful
[node 0] Cleaning up ipc directory '/var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/test-ipc-s5k_0gog'


stderr:
<frozen importlib._bootstrap>:491: RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp', which has not declared that it can run safely without the GIL. To override this behavior and keep the GIL disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0.



TEST             | STATUS    | DURATION

interface_ipc.py | ✖ Failed  | 7 s

ALL              | ✖ Failed  | 7 s (accumulated) 
Runtime: 7 s

    After: Applying this patch, when PYTHON_GIL is 0 or 1 or not set

    PYTHON_GIL=1 ./build_dev_mode/test/functional/test_runner.py interface_ipc.py
or 
PYTHON_GIL=0 ./build_dev_mode/test/functional/test_runner.py interface_ipc.py
or
./build_dev_mode/test/functional/test_runner.py interface_ipc.py

Temporary test directory at /tmp/test_runner_₿_🏃_20251105_180523
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py passed, Duration: 13 s

TEST             | STATUS    | DURATION

interface_ipc.py | ✓ Passed  | 13 s

ALL              | ✓ Passed  | 13 s (accumulated)
Runtime: 13 s
  2. DrahtBot added the label Tests on Nov 5, 2025
  3. DrahtBot commented at 11:13 PM on November 5, 2025: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33795.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Approach NACK maflcko
    Stale ACK ryanofsky

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. kevkevinpal force-pushed on Nov 6, 2025
  5. maflcko commented at 6:54 AM on November 6, 2025: member

    As mentioned in the issue, I don't see the problem that free threading solves for us. In fact, it is likely going to cause more issues that are not worth it to debug nor fix.

    Free threading could make sense if there is a heavy multithreaded production workload. However, I don't see this in the functional test, which are single-threaded, or at most double threaded?

    I'd say we should just require the GIL for all Python code. I can't see Python removing the GIL any time soon without controversy. And if they do, workarounds such as this one won't work and are not needed anyway.

  6. ryanofsky commented at 12:21 PM on November 6, 2025: contributor

    I'd be interested to know if the test passes with GIL force-disabled with PYTHON_GIL=0 or -Xgil=0. If it does, then maybe it will be trivial for a new version of pycapnp to declare it is compatible with free-threaded python and this warning will fix itself.

    In any case, I think it would probably be best just to suppress this warning with warnings.filterwarnings instead of skipping the test, since we are ok with python auto-enabling the GIL for this test, and maybe even want that. ChatGPT suggested:

    with warnings.catch_warnings():
    warnings.filterwarnings(
        "ignore",
        message=r"The global interpreter lock .* capnp\.lib\.capnp",
        category=RuntimeWarning,
        module=r"importlib\._bootstrap",
    )
    import capnp

    I think it only makes sense to skip the test if import capnp actually fails.

  7. kevkevinpal force-pushed on Nov 6, 2025
  8. kevkevinpal commented at 5:56 PM on November 6, 2025: contributor

    I went with the suggestion to suppress the warning here 4ab4fd7

    Not sure how verbose you want me to be with the message I can make it more exact to the message in the error if wanted

  9. kevkevinpal force-pushed on Nov 6, 2025
  10. DrahtBot added the label CI failed on Nov 6, 2025
  11. DrahtBot commented at 6:35 PM on November 6, 2025: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19144971669/job/54720048643</sub> <sub>LLM reason (✨ experimental): Python lint errors (ruff) halted CI, due to unused imports in test/functional/interface_ipc.py.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  12. in test/functional/interface_ipc.py:23 in 136dfde1c2 outdated 
      19 | +        warnings.filterwarnings(
  20 | +            "ignore",
  21 | +            message=r"The global interpreter lock .*",
  22 | +            category=RuntimeWarning,
  23 | +            module=r"importlib\._bootstrap",
  24 | +        )
    maflcko commented at 6:51 PM on November 6, 2025:

    not sure. This seems to go in the wrong direction. It disables a legit? warning without any reason why disabling it would be safe or is even desirable. See #33582 (comment) and #33795 (comment)

    The correct fix would be to just require the GIL

    ryanofsky commented at 7:05 PM on November 6, 2025:

    The correct fix would be to just require the GIL

    That's right, the text of the warning (from #33582) states "The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp'", so my understanding is when this test is run, the GIL is enabled and the test succeeds, except there is a spurious warning at the end which causes the test runner to fail.

    So to me it seems like a good fix is disable the warning, since we know about it and are expecting it, and there not a good reason make the test fail after it succeeds. But if you have a different fix in mind, I could imagine it making sense and would be curious to know the details.

    Seperately in 136dfde1c2284e30ecb1dfb35520f93878ec7335, I do think it would be good to deduplicate the code that detects the warning, maybe by adding a utility function, or maybe just by returning a reference to the capnp module after it is imported once.

    maflcko commented at 7:13 PM on November 6, 2025:

    The correct fix would be to just require the GIL

    That's right, the text of the warning (from #33582) states "The global interpreter lock (GIL) has been enabled to load module 'capnp.lib.capnp'", so my understanding is when this test is run, the GIL is enabled and the test succeeds, except there is a spurious warning at the end which causes the test runner to fail.

    Ok, this was not clear at all to me. It would be good to at least document, so that code-readers don't have to git blame to find the commit id, then look up the pull request, then read the pull request comments, then go back to the original issue, then read the original issue, then decide that the warning was ok to suppress.

    But if you have a different fix in mind, I could imagine it making sense and would be curious to know the details.

    I am thinking about setting PYTHON_GIL=1 in the test runner, so that tests pick it up. Also, the test framework could be adjusted to reject PYTHON_GIL=0 early in startup, to guard against manual runs outside the test runner.

    ryanofsky commented at 2:26 AM on November 7, 2025:

    re: #33795 (review)

    I am thinking about setting PYTHON_GIL=1 in the test runner, so that tests pick it up.

    This seems like a reasonable alternative, but if we take this approach I hope we only do it selectively for the tests which need it.

    I understand we may have some tests which aren't thread safe (though hopefully not too many if most tests are single-threaded) and it may be a lot easier to set PYTHON_GIL=1 than to fix them.

    But it could be a bad situation if in a few years PYTHON_GIL=1 becomes deprecated or untenable to use, and newer tests at that point wind up having the same thread safety bugs as old tests. It would seem best not to enable PYTHON_GIL=1 where it's not actually needed and develop a stronger dependency on it.

  13. maflcko changes_requested
  14. maflcko commented at 6:51 PM on November 6, 2025: member

    not sure about this

  15. DrahtBot removed the label CI failed on Nov 6, 2025
  16. in test/functional/test_framework/test_framework.py:884 in 136dfde1c2 
     879 | @@ -879,9 +880,16 @@ def skip_if_no_py_sqlite3(self):
 880 |      def skip_if_no_py_capnp(self):
 881 |          """Attempt to import the capnp package and skip the test if the import fails."""
 882 |          try:
 883 | -            import capnp  # type: ignore[import] # noqa: F401
 884 | -        except ImportError:
 885 | -            raise SkipTest("capnp module not available.")
 886 | +            with warnings.catch_warnings():
 887 | +                warnings.filterwarnings(
    ryanofsky commented at 2:15 AM on November 7, 2025:

    In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)

    Could this change to test_framework.py be reverted?

    I think it should be impossible for the warning to trigger here since the capnp module will already be imported by interface_ipc.py before this runs.

    If changing this method is actually necessary, I think it'd be good to move common code importing capnp and suppressing the warning to a function in test_framework/util.py that can do this and return a reference to the capnp module. This way the catch/filter/import code doesn't need to be repeated.

    kevkevinpal commented at 2:39 PM on November 7, 2025:

    Thanks, I updated in this commit d848b8d

    I would mention that if any other test imports capnp in the future without adding the specific logic interface_ipc.py, then it will run into the same issue. But I think this makes sense for now

  17. in test/functional/interface_ipc.py:20 in 136dfde1c2 
      16 |  try:
  17 | -    import capnp  # type: ignore[import] # noqa: F401
  18 | +    with warnings.catch_warnings():
  19 | +        warnings.filterwarnings(
  20 | +            "ignore",
  21 | +            message=r"The global interpreter lock .*",
    ryanofsky commented at 2:18 AM on November 7, 2025:

    In commit "test: Ignore error message give from python because of GIL" (136dfde1c2284e30ecb1dfb35520f93878ec7335)

    I think it's probably be good to just write the entire warning message here instead of just the beginning of the message, so it is clear what warning this code is trying to suppress.

    kevkevinpal commented at 2:36 PM on November 7, 2025:

    Thanks, I updated in this commit d848b8d

  18. ryanofsky commented at 2:38 AM on November 7, 2025: contributor

    Code review 136dfde1c2284e30ecb1dfb35520f93878ec7335. I think this is a good approach, and that suppressing the warning is the most direct fix for the test failure that avoids reducing test coverage. I left some suggestions below for avoiding code duplication and documenting the workaround better. Also think the PR title and description could be updated since the test is no longer skipped.

  19. kevkevinpal force-pushed on Nov 7, 2025
  20. kevkevinpal renamed this:
    test: skip interface_ipc if python version is freethreaded and PYTHON_GIL=0 is not set
    test: Ignore error message give from python because of PYTHON_GIL
    on Nov 7, 2025
  21. kevkevinpal commented at 2:41 PM on November 7, 2025: contributor

    Would it make sense to add a TODO to remove the warning suppression once GIL is supported in the capnp library?

  22. DrahtBot added the label CI failed on Nov 7, 2025
  23. DrahtBot commented at 2:50 PM on November 7, 2025: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19171518120/job/54805067357</sub> <sub>LLM reason (✨ experimental): (empty)</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  24. kevkevinpal force-pushed on Nov 7, 2025
  25. kevkevinpal force-pushed on Nov 7, 2025
  26. DrahtBot removed the label CI failed on Nov 7, 2025
  27. kevkevinpal force-pushed on Nov 10, 2025
  28. kevkevinpal commented at 4:34 PM on November 10, 2025: contributor

    Added a TODO to remove the warning suppression once capnp can run safely without the GIL

    ef268ee

  29. kevkevinpal force-pushed on Nov 11, 2025
  30. kevkevinpal requested review from maflcko on Nov 11, 2025
  31. kevkevinpal requested review from ryanofsky on Nov 11, 2025
  32. ryanofsky approved
  33. ryanofsky commented at 7:13 PM on November 13, 2025: contributor

    Code review ACK ef268ee288c04a31dd802c4bb729a0c2e6e64786. IMO, this is the most direct fix possible for #33582: Python is warning about something which we don't care about, so we disable the warning. I think this is a better fix than skipping the test or forcing the GIL to be enabled long-term. It can be reverted later if pycapnp is updated to work without the GIL, or if we decide we do want to turn on the GIL explicitly.

  34. DrahtBot added the label Needs rebase on Jan 13, 2026
  35. test: Ignore error message give from python because of GIL eedea77d05
  36. kevkevinpal force-pushed on Mar 13, 2026
  37. kevkevinpal commented at 4:21 PM on March 13, 2026: contributor

    Rebased this PR to eedea77

  38. DrahtBot removed the label Needs rebase on Mar 13, 2026
  39. sedited commented at 1:23 PM on March 19, 2026: contributor

    @maflcko do you want to give this another look?

  40. maflcko commented at 1:45 PM on March 19, 2026: member

    Yeah, I think I am not affected and I am not sure who is even affected.

    No objection, but I think the only ones affected are the ones shooting themselves in the foot: #33582 (comment)

    Also, I don't really understand the pull description. It doesn't list any error message or warning related to the changes here. Instead it lists an unrelated warning and a passing test, the meaning of which is unclear:

    Temporary test directory at /tmp/test_runner_₿_🏃_20251105_180523
WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py passed, Duration: 13 s

TEST             | STATUS    | DURATION

interface_ipc.py | ✓ Passed  | 13 s

ALL              | ✓ Passed  | 13 s (accumulated)
Runtime: 13 s

    ~0 on this, but I don't mind other people taking a look here

  41. kevkevinpal commented at 2:25 PM on March 19, 2026: contributor

    Thanks for the review!

    Also, I don't really understand the pull description. It doesn't list any error message or warning related to the changes here. Instead it lists an unrelated warning and a passing test, the meaning of which is unclear:

    Sorry about that, I updated the description, hopefully it makes more sense now for new readers

  42. maflcko commented at 3:20 PM on March 19, 2026: member

    Approach NACK. Not a blocker, but I don't like the direction of this change, as already explained several times.

    We already have more than enough intermittent issues to deal with right now. Somehow pretending that we support no-gil, want to debug issues in it, have them filed, tracked, and fixed, seems the wrong direction.

    There isn't any benefit in doing so right now. In fact, there are immediate downsides (https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3386064377, #33795 (comment)).

    The only benefit seems to be in a made-up future where gil is deprecated (https://github.com/bitcoin/bitcoin/pull/33795#discussion_r2501470819)

    Absent any indication of this happening, it doesn't seem worth it to spend time on this. And by the time it does happen, hopefully there will be tooling around, like a tsan for python to deal/debug it.

    The correct fix would be to temporarily force PYTHON_GIL=1 for all tests and then properly re-visit this when there is an actual need or benefit.

  43. kevkevinpal commented at 6:27 PM on March 19, 2026: contributor

    Thanks for the review!

    The correct fix would be to temporarily force PYTHON_GIL=1 for all tests and then properly re-visit this when there is an actual need or benefit.

    That makes sense. I opened a PR doing this #34869

    I can keep this PR open for now if there is objection to default using PYTHON_GIL=1 for all tests

  44. kevkevinpal commented at 2:27 PM on March 21, 2026: contributor

    Closing this PR because this was merged instead #34869

    Feel free to reopen if we ever want to include a change like this

  45. kevkevinpal closed this on Mar 21, 2026
Contributors
kevkevinpalDrahtBotmaflckoryanofskysedited


maflcko

Labels
Tests
Linked (view graph)
#28722 Multiprocess tracking issue#33582 test: pycapnp doesn't support free threaded Python
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-15 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me