[core] Fix bugs in data locality#24698
Conversation
clarkzinzow
left a comment
clarkzinzow
left a comment
There was a problem hiding this comment.
LGTM if tests pass and nightly test looks good
| // it as local in the in-memory store so that the data locality policy | ||
| // will choose the right raylet for any queued dependent tasks. | ||
| const auto pinned_at_raylet_id = NodeID::FromBinary(worker_addr.raylet_id()); | ||
| reference_counter_->UpdateObjectPinnedAtRaylet(object_id, pinned_at_raylet_id); |
There was a problem hiding this comment.
Ah nice ordering catch!
| auto node_ids = it->second.locations; | ||
| if (!it->second.spilled_node_id.IsNil()) { | ||
| node_ids.emplace(it->second.spilled_node_id); | ||
| } |
| ray.wait(xs, num_returns=len(xs), fetch_local=False) | ||
| for i, x in enumerate(xs): | ||
| task = check_locality.remote(x) | ||
| print(i, x, task) |
There was a problem hiding this comment.
Nit: Should remove debug print.
|
Still seeing unexpected transfers when there is heavy task load, but this does seem to have fixed the problem in 10TB sort where the head node raylet was coming under heavy load. Before PR: After PR: Note task throughput (green lline) is very low towards the righthand side of the graph in the before version. |
|
Can this affect |
|
Hmm possibly, but it might just be that the test suite is too large, @mwtian? |
|
That is possible. I guess the test needs to be splitted. |
|
Or is it possible that reconstruction becomes slower? It seems timeouts of 900s happen at or after |
|
OK, I'll try to split test_reconstruction after the revert is merged. |
|
Sounds good. If you verified that the slowdown is harmless, I can split the tests too. |
…ray-project#25035)" This reverts commit 916c679.
Redo for PR #24698: This fixes two bugs in data locality: When a dependent task is already in the CoreWorker's queue, we ran the data locality policy to choose a raylet before we added the first location for the dependency, so it would appear as if the dependency was not available anywhere. The locality policy did not take into account spilled locations. Added C++ unit tests and Python tests for the above. Split test_reconstruction to avoid test timeout. I believe this was happening because the data locality fix was causing extra scheduler load in a couple of the reconstruction stress tests.
4 participants
Why are these changes needed?
This fixes two bugs in data locality:
Added C++ unit tests and Python tests for the above.
Related issue number
Fixes #24267.
Checks
scripts/format.shto lint the changes in this PR.