-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade free-threading CI to run with pytest-freethreaded instead of pytest-xdist #30007
Comments
Discussing with @lesteve we have a problem with test fixture that use on Maybe there is a problem of scoping the fixture correctly, and somehow telling pytest to run the fixture in the thread running the test. |
I think that for a function-scoped fixture (the default fixture scope), it would make sense to ensure that the parallel runner runs the fixture on the same thread as the test function it-self. Shall we open an issue upstream in the issue tracker of the pytest-freethreaded repo? |
Here is a somewhat simple test file that reproduces the issue. Note this happens on vanilla CPython 3.13 not only free-threaded CPython 3.13. As you can tell from the output, the fixture is run in the main thread and the test function in another thread (threadpool created by the pytest plugin). # test_simple.py
import threading
import pytest
thread_local = threading.local()
@pytest.fixture
def change_thread_local():
print("inside fixture", threading.current_thread())
setattr(thread_local, "value", 1)
yield
def test_simple(change_thread_local):
print("inside function", threading.current_thread())
assert getattr(thread_local, "value", None) == 1
Output:
|
ping @andfoy |
Hi, from the SciPy side, We've been testing using It also fully supports fixtures |
I tried @lesteve's reproducer with
The function-fixture is executed in a different thread (the main thread) than the thread used to execute the test function. |
I will open an issue in https://github.com/tonybaloney/pytest-freethreaded with Loic's reproducer. |
@andfoy let me know if you want me to open a similar issue in https://github.com/Quansight-Labs/pytest-run-parallel. |
I think the issue at hand is that a separate threaded fixture is required here, which should be different from a global, pytest fixture. However, from a concurrency/distribution/parallelism point of view, each thread should get its own local atomic state, which should be independent from each other. As pytest fixtures are spawned globally from the main thread, the case exposed by @lesteve is the expected behaviour rather than a bug. In order to fix this, there could be 2 different avenues:
I support going for the second option rather than the first one, since it invites developers to think about concurrency, parallel and concurrent constructs explicitly, as opposed to having to rely on a separate machinery to (apparently) do the job without thinking about what is going on underneath. This is of uttermost importance if free-threaded is going to be the future of Python at some point. |
It's worth keeping in mind as well that Pytest itself is not thread safe and I don't think the maintainers are particularly open to making it thread safe. More on this under this page: https://docs.pytest.org/en/stable/explanation/flaky.html |
I guess this is towards option 1. but pytest-parallel (archived and no longer maintained) seems run the fixture and the test function in the same thread, see tonybaloney/pytest-freethreaded#17 (comment) for more details. |
Thanks for your feedback @andfoy.
I am trying to see what that would entail for scikit-learn. @lesteve I tried to grep our source code to find all the occurrences of https://github.com/search?q=repo%3Ascikit-learn%2Fscikit-learn+enable_slep006&type=code Each of those tests would need to have its body indented under a So some form of thread-local fixtures would be nice in my opinion. |
Or maybe we could just write our own test function decorator instead of using a pytest fixture. EDIT: the following might work for us: import functools
import sklearn
def enable_metadata_routing(func):
@functools.wraps(func)
def wrapper(*args, **kwargs):
with sklearn.config_context(enable_metadata_routing=True):
return func(*args, **kwargs)
return wrapper I will open a quick PR. EDIT 2: actually context managers can directly be used as decorators. So the following works and is even cleaner: @sklearn.config_context(enable_metadata_routing=True)
def test_function():
pass |
Note that we will still have problems with non scikit-learn fixtures such as Furthermore, the link to https://docs.pytest.org/en/stable/explanation/flaky.html posted by @ngoldbaum above also mentions that EDIT: I confirm that tests that use |
Maybe we need a way to mark some tests as inherently non thread-safe (e.g. if they rely on Thread parallel pytest runners such as pytest-run-parallel or pytest-freethreaded would then make sure that all those tests are grouped to be run sequentially (in the same thread, possibly even the main thread). |
For warnings it's worse than that, the whole warnings module I was just talking to @lysnikolaou about this today and the issue is that making the warning module thread safe and scalable is very tricky. |
@ngoldbaum what do you think of my proposal above to introduce a |
|
We could introduce a |
Thanks, @andfoy, I confirm that using Still, I think |
@andfoy Back to the original problem of function-scoped fixture. I found a problem with a simple use of the tonybaloney/pytest-freethreaded#17 (comment) This fixture provides valid test isolation only if it is run around each function call within the same thread as the one actually executing the test. Otherwise, one would need an extra layer of thread protection to be able to correctly use it with a thread-based parallel execution plugin. That would be too invasive in my opinion. |
I started #30041 to investigate what would be needed to get our test suite to pass with parallel thread execution. It's quite tedious to manually mark all the I think we instead need:
|
For temporary directories, I think you should either migrate to a function that doesn't use temporary directories with a specific name, or prepend the name of the directory with e.g. a UUID to make the directory name unique. |
But the point of the Granted, the import pytest
@pytest.mark.parametrize("value", list("abcd"))
def test_simple(tmp_path, value):
file_path = tmp_path / "file.txt"
assert not file_path.exists()
file_path.write_text(value)
read_value = file_path.read_text()
assert read_value == value
|
I guess I don't see why you need to have a specific name that's specified in the test and why you want to go out of your way to keep that. Since you're running the same test in many threads and are implicitly using global state (the file system), then of course that's not thread safe. IMO you should update your testing code to not use the global state, rather than expect the testing code to hide the global state from you. |
Also, I just created Quansight-Labs/free-threaded-compatibility#92 to mention these issues around writing thread-safe tests in the guide. |
But |
I want to double-check this, since I think if you were running the same test many times in parallel with xdist you'd have the same issue. |
import pytest
@pytest.mark.parametrize("value", list("abcd"))
def test_simple(tmp_path, value):
file_path = tmp_path / "file.txt"
assert not file_path.exists()
file_path.write_text(value)
read_value = file_path.read_text()
assert read_value == value
|
As @ngoldbaum mentioned, tests that use This goes in line with the philosophy of being aware of concurrent computing, as opposed to having a complex and opaque machinery that tries to abstract the problem away. I understand that going through each test in the library takes a lot of time, but doing so will ensure that each test function is thread-safe (even if the solution sometimes implies adding locks) and it will future-proof it for future contributors, as adding a CI (for example), will force them to not use global state patterns. It is difficult to get away from thinking in global state, which is common in single-threaded imperative or object oriented computing. But free-threaded deals with a different paradigm, and requires different abstractions and patterns |
But it's sad to require the test to handle the thread-isolation by itself. It would be a much better separation of concern if those fixtures or the runner could handle the thread-aware isolation by them-selves. It goes against the DRY principle. We could probably write a |
I suppose it's possible to maintain a per-project test utility wrapper such as: import pytest
import threading
def thread_local_path(path):
local_path = path / f"thread-{threading.get_ident()}"
local_path.mkdir()
return local_path
@pytest.mark.parametrize("value", list("abcd"))
def test_simple(tmp_path, value):
file_path = thread_local_path(tmp_path) / "file.txt"
assert not file_path.exists()
file_path.write_text(value)
read_value = file_path.read_text()
assert read_value == value |
There is a new kid on the block that should help us find out whether scikit-learn and its dependencies can be reliably considered free-threading compatible:
https://pypi.org/project/pytest-freethreaded/
Let's try to adopt it in scikit-learn.
Here is a possible plan:
pytest-freethreaded
.Any comments @lesteve @jeremiedbb @ngoldbaum?
EDIT: anyone interested in getting hands on the first item can find this resource useful:
https://py-free-threading.github.io/
EDIT 2: there is also the pytest-run-parallel plugin that can serve a similar purpose.
The text was updated successfully, but these errors were encountered: