-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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] Fix bugs in data locality #24698
[core] Fix bugs in data locality #24698
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); | ||
} |
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.
Nice!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. Note task throughput (green lline) is very low towards the righthand side of the graph in the before version. |
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.
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.sh
to lint the changes in this PR.