-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Fix Client Table #2880
Fix Client Table #2880
Conversation
@@ -522,8 +524,9 @@ def client_table(self): | |||
client.ResourcesTotalCapacity(i) | |||
for i in range(client.ResourcesTotalLabelLength()) | |||
} | |||
node_info.append({ | |||
"ClientID": ray.utils.binary_to_hex(client.ClientId()), | |||
client_id = ray.utils.binary_to_hex(client.ClientId()) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
}) | ||
return node_info | ||
} | ||
return list(node_info.values()) |
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 a list is probably the right structure to return, though I'm not sure. A map from client_id
to info doesn't make sense because client_id
s are not really meaningful to users. A map from node_ip_address
to list of clients could make sense, though in the vast majority of cases each list will contain a single element.
I think this is fine for now. This should work in the setting where we have multiple raylets per node, right?
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.
Yes, this works in the setting where we have multiple raylets per node (although it doesn't work when the raylets per node are removed ;) #2878)
I think the client_id -> info
mapping is actually fine, but I'm also OK with leaving as is. In calling client_table
, I don't expect a list of node_ip_addresses; I expect some data structure that has len() == num clients.
Test FAILed. |
Test FAILed. |
OK I added a light test for this using subprocess to start ray; this may only work in xray? and may also require Is there a better way to achieve what I'm trying to do? |
Test PASSed. |
Test PASSed. |
Test PASSed. |
python/ray/test/test_global_state.py
Outdated
|
||
|
||
@ray.remote(num_cpus=1) | ||
def cpu_task(seconds): | ||
time.sleep(seconds) | ||
|
||
|
||
def _get_raylet_pid(raylet_socket): |
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.
Hey @robertnishihara, is there a better way of getting the Raylet PID?
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 good to me, pending tests passing.
Test FAILed. |
jenkins, retest this please |
Test PASSed. |
Hides append-only log in the Python implementation.
Should partially address #2852.
TODO: