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

kevkevinpal · 2025-11-05T23:13:29Z

Description

I was able to reproduce this issue #33582
We should ignore the warning message if PYTHON_GIL is not set, instead of the test being abruptly stopped

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
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

DrahtBot · 2025-11-05T23:13:36Z

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
#33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
#33965 (mining: fix -blockreservedweight shadows IPC option by Sjors)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33819 (mining: getCoinbase() returns struct instead of raw tx by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2025-11-06T06:54:50Z

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.

ryanofsky · 2025-11-06T12:21:48Z

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.

kevkevinpal · 2025-11-06T17:56:50Z

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

DrahtBot · 2025-11-06T18:35:55Z

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

Hints

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.

maflcko

not sure about this

maflcko · 2025-11-06T18:51:11Z

test/functional/interface_ipc.py

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


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

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.

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.

re: #33795 (comment)

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.

ryanofsky

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.

test/functional/test_framework/test_framework.py

test/functional/interface_ipc.py

ryanofsky · 2025-11-07T02:26:14Z

test/functional/interface_ipc.py

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


re: #33795 (comment)

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.

kevkevinpal · 2025-11-07T14:41:01Z

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

DrahtBot · 2025-11-07T14:50:30Z

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

Hints

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.

kevkevinpal · 2025-11-10T16:34:19Z

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

ef268ee

ryanofsky

Code review ACK ef268ee. 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.

DrahtBot · 2026-01-13T13:17:40Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the Tests label Nov 5, 2025

kevkevinpal force-pushed the skipIfGILTestError branch from 8dd16d9 to 516f87d Compare November 6, 2025 02:59

kevkevinpal force-pushed the skipIfGILTestError branch from 516f87d to 4ab4fd7 Compare November 6, 2025 17:54

kevkevinpal force-pushed the skipIfGILTestError branch from 4ab4fd7 to 136dfde Compare November 6, 2025 18:35

DrahtBot added the CI failed label Nov 6, 2025

maflcko suggested changes Nov 6, 2025

View reviewed changes

DrahtBot removed the CI failed label Nov 6, 2025

ryanofsky reviewed Nov 7, 2025

View reviewed changes

kevkevinpal force-pushed the skipIfGILTestError branch from 136dfde to d848b8d Compare November 7, 2025 14:33

kevkevinpal changed the title ~~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 Nov 7, 2025

DrahtBot added the CI failed label Nov 7, 2025

kevkevinpal force-pushed the skipIfGILTestError branch 2 times, most recently from 1f2f5b8 to e075fe7 Compare November 7, 2025 17:28

DrahtBot removed the CI failed label Nov 7, 2025

DrahtBot mentioned this pull request Nov 8, 2025

mining: getCoinbase() returns struct instead of raw tx #33819

Merged

kevkevinpal force-pushed the skipIfGILTestError branch from e075fe7 to 5b7778c Compare November 10, 2025 16:20

test: Ignore error message give from python because of GIL

ef268ee

kevkevinpal force-pushed the skipIfGILTestError branch from 5b7778c to ef268ee Compare November 11, 2025 13:49

kevkevinpal requested review from maflcko and ryanofsky November 11, 2025 13:50

ryanofsky approved these changes Nov 13, 2025

View reviewed changes

DrahtBot mentioned this pull request Nov 18, 2025

mining: add requestedOutputs field, e.g. for merged mining #33890

Closed

ryanofsky mentioned this pull request Nov 24, 2025

Multiprocess tracking issue #28722

Open

This was referenced Nov 29, 2025

refactor: disentangle miner startup defaults from runtime options #33966

Draft

mining: fix -blockreservedweight shadows IPC option #33965

Open

mining: add getMemoryLoad() and track template non-mempool memory footprint #33922

Open

DrahtBot mentioned this pull request Dec 31, 2025

mining: add cooldown to createNewBlock() immediately after IBD #34184

Open

DrahtBot added the Needs rebase label Jan 13, 2026

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

Are you sure you want to change the base?

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

Conversation

kevkevinpal commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

When PYTHON_GIL is 0 or 1 or not set

Uh oh!

DrahtBot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

maflcko commented Nov 6, 2025

Uh oh!

ryanofsky commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevkevinpal commented Nov 6, 2025

Uh oh!

DrahtBot commented Nov 6, 2025

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

kevkevinpal commented Nov 7, 2025

Uh oh!

DrahtBot commented Nov 7, 2025

Uh oh!

kevkevinpal commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kevkevinpal commented Nov 5, 2025 •

edited

Loading

When `PYTHON_GIL` is `0` or `1` or not set

DrahtBot commented Nov 5, 2025 •

edited

Loading

ryanofsky commented Nov 6, 2025 •

edited

Loading

kevkevinpal commented Nov 10, 2025 •

edited

Loading

ryanofsky left a comment •

edited

Loading