test: Ignore error message give from python because of PYTHON

kevkevinpal commented at 11:13 pm on November 5, 2025: contributor

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

 0PYTHON_GIL=1 ./build_dev_mode/test/functional/test_runner.py interface_ipc.py
 1or 
 2PYTHON_GIL=0 ./build_dev_mode/test/functional/test_runner.py interface_ipc.py
 3or
 4./build_dev_mode/test/functional/test_runner.py interface_ipc.py
 5
 6Temporary test directory at /tmp/test_runner_₿_🏃_20251105_180523
 7WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!
 8Remaining jobs: [interface_ipc.py]
 91/1 - interface_ipc.py passed, Duration: 13 s
10
11TEST             | STATUS    | DURATION
12
13interface_ipc.py | ✓ Passed  | 13 s
14
15ALL              | ✓ Passed  | 13 s (accumulated)
16Runtime: 13 s

DrahtBot added the label Tests on Nov 5, 2025

DrahtBot commented at 11:13 pm on November 5, 2025: contributor

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33819 (mining: add getCoinbase() 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.

kevkevinpal force-pushed on Nov 6, 2025

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.

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:

0with warnings.catch_warnings():
1    warnings.filterwarnings(
2        "ignore",
3        message=r"The global interpreter lock .* capnp\.lib\.capnp",
4        category=RuntimeWarning,
5        module=r"importlib\._bootstrap",
6    )
7    import capnp

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

kevkevinpal force-pushed on Nov 6, 2025

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

kevkevinpal force-pushed on Nov 6, 2025

DrahtBot added the label CI failed on Nov 6, 2025

DrahtBot commented at 6:35 pm on November 6, 2025: contributor

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

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.

in test/functional/interface_ipc.py:24 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.

maflcko changes_requested

maflcko commented at 6:51 pm on November 6, 2025: member

not sure about this

DrahtBot removed the label CI failed on Nov 6, 2025

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

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

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.

kevkevinpal force-pushed on Nov 7, 2025

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

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?

DrahtBot added the label CI failed on Nov 7, 2025

DrahtBot commented at 2:50 pm on November 7, 2025: contributor

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

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 force-pushed on Nov 7, 2025

DrahtBot removed the label CI failed on Nov 7, 2025

kevkevinpal force-pushed on Nov 10, 2025

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

test: Ignore error message give from python because of GIL ef268ee288

kevkevinpal force-pushed on Nov 11, 2025

kevkevinpal requested review from maflcko on Nov 11, 2025

kevkevinpal requested review from ryanofsky on Nov 11, 2025

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

Description

When PYTHON_GIL is 0 or 1 or not set

Code Coverage & Benchmarks

Reviews

Conflicts

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