-
Notifications
You must be signed in to change notification settings - Fork 11
ENH: Free-threading support #330
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
Conversation
efde2aa
to
bf5b21c
Compare
@@ -23,6 +23,7 @@ classifiers = [ | |||
"Programming Language :: Python :: 3.11", | |||
"Programming Language :: Python :: 3.12", | |||
"Programming Language :: Python :: 3.13", | |||
"Programming Language :: Python :: Free Threading :: 3 - Stable", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole library has 1 stateful class, at
, which has trivial state. So no point adding tests beyond those of pytest-run-parallel
.
Where we are thread-unsafe, in lazy_xp_function
, it is clearly documented, so I believe this should qualify for the Stable badge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where we are thread-unsafe, in lazy_xp_function, it is clearly documented, so I believe this should qualify for the Stable badge?
@ngoldbaum perhaps you could confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Guido!
src/array_api_extra/testing.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would appreciate a second review from someone for the changes in this file. All looks fine to me from a non-expert perspective, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @rgommers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just browsed the diff, looks good at a high level. I won't be able to do a line-by-line review, but that's probably also not necessary - and Nathan also just looked at the test machinery here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good then, happy to trust Guido on the details 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ngoldbaum perhaps you could confirm?
Makes sense to me!
I didn't look at the pytest changes in detail but I looked it over to get a feel for what's happening here and it makes sense to me. I left one comment inline but it's not actionable by Guido. More a note to us that we should think about how to allow this testing pattern to work with pytest-run-parallel.
) | ||
if self.like(Backend.DASK, Backend.JAX): | ||
# Monkey-patched by lazy_xp_function | ||
marks.append(pytest.mark.thread_unsafe) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is unavoidable but it's a little bit of a bummer. Makes me think as a long-term item we should figure out how to get e.g. mock
or other monkeypatching to work in a thread-safe manner.
pytest-run-parallel
in CImonkeypatch
parameter oflazy_xp_function
in favour of explicitly tagging Dask and JAX withpytest.mark.thread-unsafe
, while leaving the other backends unmarkedlazy_xp_function
is incompatible withpytest-run-parallel
#307Matching scipy PR:
xp
tests scipy/scipy#23143Demo