-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[Refactor] fix ruff rule B039: mutable-contextvar-default #49854
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: LeoLiao123 <[email protected]>
Signed-off-by: LeoLiao123 <[email protected]>
@MortalHappiness PTAL |
Signed-off-by: LeoLiao123 <[email protected]>
python/ray/serve/context.py
Outdated
@@ -195,7 +195,7 @@ def _set_request_context( | |||
"""Set the request context. If the value is not set, | |||
the current context value will be used.""" | |||
|
|||
current_request_context = _serve_request_context.get() | |||
current_request_context = _serve_request_context.get() or _RequestContext() |
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.
It's better to follow the reference and check it with if current_request_context is None
, as None
is not the only value with a False
boolean value in Python.
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 for the suggestion. Updated to check with is None
.
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.
Still need to update other files that use this variable. You can create an utility function like
def get_serve_request_context():
if _serve_request_context.get() is None:
_serve_request_context.set(_RequestContext())
return serve_request_context.get()
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've updated all related files and implemented the utility function for get_serve_request_context()
.
Signed-off-by: LeoLiao123 <[email protected]>
Signed-off-by: LeoLiao123 <[email protected]>
Signed-off-by: LeoLiao123 <[email protected]>
Signed-off-by: LeoLiao123 <[email protected]>
Signed-off-by: LeoLiao123 <[email protected]>
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.
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.
Looks great pending my one comment -- thanks for the fix!
python/ray/serve/context.py
Outdated
@DeveloperAPI | ||
def get_serve_request_context(): |
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.
@DeveloperAPI | |
def get_serve_request_context(): | |
def _get_serve_request_context(): |
this is an internal API, not public at the moment
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.
Fixed, thanks for the review.
…quest_context as an internal API Signed-off-by: LeoLiao123 <[email protected]>
Why are these changes needed?
Replace mutable default value with None to prevent shared state across ContextVar.get() calls
ref : https://docs.astral.sh/ruff/rules/mutable-contextvar-default/
Related issue number
#47991
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.