[Client] Make Client_Hook per-thread
#16731
Conversation
|
I think this should resolve #16743 as well cc @matthewdeng |
ijrsvt
force-pushed
the
make-client-thread-safe
branch
from
9326aba
to
9641530
Compare
|
Ugh, the test issues from earlier were due to my erroneous use of I added a comment that |
| return out | ||
|
|
||
|
|
||
| def _explicitly_enable_client_mode(): | ||
| """Force client mode to be enabled. | ||
| NOTE: This should not be used in tests, use `enable_client_mode`. | ||
| """ | ||
| global client_mode_enabled |
There was a problem hiding this comment.
What is the difference between client mode enabled and client hook enabled?
There was a problem hiding this comment.
I think "client mode enabled" is a global setting saying whether ray client is enabled and "client hook enabled" is an internal toggle used by the code to turn on/off delegating ray apis to the client stub.
It could help to add a comment explaining the meanings to this file.
There was a problem hiding this comment.
Yep as @DmitriGekhtman said.
I added two comments explaining these!!
I also renamed them:
client_mode_enabled->is_client_mode_enabled(making it clear that this is answering the question of 'is client mode currently enabled'.client_hook_enabled->_client_hook_status_on_thread(making it clear that it is thread specific).
| def _get_client_hook_enabled(): | ||
| global _client_hook_enabled | ||
| if not hasattr(_client_hook_enabled, "val"): | ||
| _client_hook_enabled.val = True |
There was a problem hiding this comment.
Just curious - is this right: when you start a new thread, its copy of _client_hook_enabled doesn't have val set, hence the need for this function?
There was a problem hiding this comment.
@DmitriGekhtman Yep!
|
liiiint |
Hmmm, I knew it was too good to be true when local formatting didn't do anything 😆 |
Why are these changes needed?
_client_hook_enabledto a thread-local variableRelated issue number
Checks
scripts/format.shto lint the changes in this PR.