-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
[core][refactor] Move node_stats_to_dict
to utils.py
to avoid importing unnecessary modules
#51187
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: kaihsun <[email protected]>
python/ray/dashboard/utils.py
Outdated
@@ -358,6 +358,31 @@ def address_tuple(address): | |||
return ip, int(port) | |||
|
|||
|
|||
def node_stats_to_dict(message): |
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.
add type hints please
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.
added 75d4f5a
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 type hint is derived from the return type of node_stats
.
ray/python/ray/_private/internal_api.py
Line 92 in 2f8684f
def node_stats( |
finally: | ||
message.core_workers_stats.extend(core_workers_stats) |
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.
do you know why this is in a finally block? very strange pattern
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 function first clears the field
core_workers_stats
from the message. - It converts the message into a dict and constructs
result["coreWorkersStats"]
. finally
: if step (2) throws an exception, the fieldcore_workers_stats
is set back on the message to avoid side effects.
Signed-off-by: kaihsun <[email protected]>
Why are these changes needed?
node_stats_to_dict
is used by bothmemory_utils.py
andnode_head.py
, andmemory_utils.py
importsray.dashboard.modules.node.node_head
. It's better to avoid having a "utils" module import other internal Python modules to avoid nested import or potential circular import.Related issue number
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.