-
Notifications
You must be signed in to change notification settings - Fork 16
Feature: Threadsafe RcParams #383
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
d8d95b7 to
69a623d
Compare
|
Need to do some final checks to see how this interacts with the |
beckermr
left a comment
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 think there may be a bug in the logic.
ultraplot/internals/rcsetup.py
Outdated
| if key is not None: | ||
| # Store in both thread-local storage and main dictionary | ||
| self._local.changes[key] = value | ||
| dict.__setitem__(self, key, value) |
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.
This block doesn't make sense to me. I think we need to test if we're in the context manager block and then only make a local change if we are. Otherwise threads will make changes to the global settings which could interfere with each other.
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 am leaning towards moving this logic (thread safety) inside the Configurator we both need to control ultraplot's rc and matplotlib's rc making this PR a bit moot and #384 an essential step in this direction.
Think you are right. I believe I fixed it now. I updated the docstring to reflect the intent of the block. The idea is that we have a main thread which contains the correct information, but threads can locally modify the rc params without affecting the main thread. I will document this as we are not resolving any conflicting settings that happens on the threads themselves, the main thread contains the "correct" information. |
|
hmm the threadsafety wrapping broke something -- will address this when I have more time. |
This PR makes our configuration system thread-safe and adds support for thread-local storage. This is particularly important when running tests in parallel (e.g., with pytest-xdist), where each test thread needs to modify configuration parameters without affecting other threads.
I started playing around with a recent PR that would be merged in the future(matplotlib/pytest-mpl#242 (comment)). This PR would prep for making it threadsafe -- speeding up local development. GHA has 2 threads per worker so it could be beneficial there, and otherwise not hurt.
The Problem
Currently, when multiple threads try to access or modify configuration parameters at the same time, we can get race conditions and data corruption. Also, there's no way for a thread to make temporary changes that are automatically cleaned up when the thread exits.
Proposed fix
The PR implements a thread-safe version of
_RcParamsthat:threading.RLock) to prevent race conditionsAdded tests
test_rcparams_thread_safety