Skip to content
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

Switch to a new thread-safe utility for catching warnings. #25626

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

hawkinsp
Copy link
Collaborator

The Python warnings.catch_warnings() functionality is not thread-safe (https://py-free-threading.github.io/porting/#the-warnings-module-is-not-thread-safe), so we cannot use it during tests that use free-threading. This change introduces a private warnings test helper (test_warning_util.py), which hooks the CPython warning infrastructure and uses it to implement thread-safe warnings infrastructure.

This requires a handful of small modifications to tests to remove direct uses of the warnings module. We also sadly have to delete one TPU test that checks for a warning raised on another thread; there's no easy way for us to catch that in a thread-safe way, but that test seems like overkill anyway.

@hawkinsp hawkinsp requested a review from jakevdp December 20, 2024 02:32
@hawkinsp
Copy link
Collaborator Author

There are some test failures because there are still some uses of warnings.catch_warnings in non-test parts of JAX. I'll need to clean those up (they will certainly break in free-threading mode).

jax/_src/test_util.py Outdated Show resolved Hide resolved
jax/_src/test_util.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tests/warnings_util_test.py Show resolved Hide resolved
@nascheme
Copy link

FYI: python/cpython#128300 : Add thread-safe context manager to "warnings" module

This is going to take some time to get fixed in CPython but it might be a good idea to make a similar API so that you can switch back to the CPython version in the future, assuming it gets fixed properly. If you have any feedback on my PR, I would be happy to hear about it. My main concerns were to not break existing non-threaded non-async code that is happily using catch_watchings() and add an API that can be used in a backwards compatible way (for code that wants to support both old and newer versions of Python).

@hawkinsp hawkinsp force-pushed the warnings2 branch 2 times, most recently from 8d0bad1 to 2351fbb Compare January 9, 2025 16:37
@hawkinsp
Copy link
Collaborator Author

hawkinsp commented Jan 9, 2025

FYI: python/cpython#128300 : Add thread-safe context manager to "warnings" module

This is going to take some time to get fixed in CPython but it might be a good idea to make a similar API so that you can switch back to the CPython version in the future, assuming it gets fixed properly. If you have any feedback on my PR, I would be happy to hear about it. My main concerns were to not break existing non-threaded non-async code that is happily using catch_watchings() and add an API that can be used in a backwards compatible way (for code that wants to support both old and newer versions of Python).

Thanks! I'm excited to see that this is being fixed properly upstream.

Given I just added a fallback path for non-threaded cases that calls the upstream utilities, it shouldn't be a big lift to switch back to the upstream utilities for the threaded cases also when a thread-safe alternative exists.

I'd assume that your improved warnings will not land until Python 3.14, correct?

jax/_src/test_warning_util.py Outdated Show resolved Hide resolved
@google-ml-butler google-ml-butler bot added kokoro:force-run pull ready Ready for copybara import and testing labels Jan 9, 2025
@hawkinsp hawkinsp force-pushed the warnings2 branch 2 times, most recently from 253a908 to be66011 Compare January 9, 2025 16:43
The Python warnings.catch_warnings() functionality is not thread-safe (https://py-free-threading.github.io/porting/#the-warnings-module-is-not-thread-safe), so we cannot use it during tests that use free-threading. This change introduces a private warnings test helper (test_warning_util.py), which hooks the CPython warning infrastructure and uses it to implement thread-safe warnings infrastructure.

This requires a handful of small modifications to tests to remove direct uses of the warnings module. We also sadly have to delete one TPU test that checks for a warning raised on another thread; there's no easy way for us to catch that in a thread-safe way, but that test seems like overkill anyway.
@copybara-service copybara-service bot merged commit 7718ac5 into jax-ml:main Jan 9, 2025
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull ready Ready for copybara import and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants