This repository was archived by the owner on Nov 17, 2023. It is now read-only.
Fix segfaults and tests for distributed kvstore #8207
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ull, and handling the case of a nullptr
Member
|
PR checklist is for reflecting the progress and important aspects of the change. Please maintain. |
piiswrong
reviewed
Oct 10, 2017
src/kvstore/comm.h
Outdated
|
|
||
| void BroadcastRowSparse(int key, const NDArray& src, | ||
| const std::vector<std::pair<NDArray*, NDArray>>& dst, | ||
| const std::vector<std::pair<NDArray*, NDArray>> dst, |
Member
Author
There was a problem hiding this comment.
Reverted after verifying that passing by reference doesn't cause scope issues
piiswrong
reviewed
Oct 10, 2017
| for (size_t i = 0; i < num_vals; i++) { | ||
| auto &row_id = target_val_rowids[i].second; | ||
| NDArray indices = row_id.Copy(pinned_ctx_); | ||
| NDArray indices(row_id.shape(), pinned_ctx_, false, mshadow::kInt64); |
Member
There was a problem hiding this comment.
This makes sure rsp_pull(val, rowid) can accept rowid with dtype other than int64.
piiswrong
reviewed
Oct 10, 2017
src/kvstore/kvstore_dist.h
Outdated
| // This shouldn't affect training of networks though because training involves | ||
| // a sequence of push, pull, then push. This imposes ordering that the | ||
| // second push happens after the first pull, and the pull happens after first push. | ||
| send_buf = merged; // avoid memory copy |
piiswrong
reviewed
Oct 10, 2017
| // at this point, later functions may access the indices variable while copy happens | ||
| mshadow::Copy(recv_buf->aux_data(kIdx).FlatTo1D<cpu, int64_t>(), | ||
| indices_data.FlatTo1D<cpu, int64_t>()); | ||
| CHECK_NOTNULL(ps_worker_)->ZPull(pskv.keys, vals, &pskv.lens, kRowSparsePushPull, |
Member
There was a problem hiding this comment.
We want to make sure Copy(indices) is done before ZPull() is completed, so that we don't broadcast with garbage indices
piiswrong
reviewed
Oct 10, 2017
| pskv.keys.push_back(ps_key); | ||
| pskv.lens.push_back(unit_len); | ||
| pskv.size += unit_len; | ||
| if (offsets) { |
Member
There was a problem hiding this comment.
offset might be nullptr when gradients are complete zeros
Member
Author
|
Addressed comments and made a few changes |
Update docs
Update comment
rahul003
changed the title
mbaijal
pushed a commit
to mbaijal/incubator-mxnet
that referenced
this pull request
Oct 12, 2017
* fix segfault in kvstore_dist for row sparse by: moving copy before zpull, and handling the case of a nullptr * fix indent, and bring back references * Update kvstore.py Update docs * Update kvstore_dist.h Update comment * Update kvstore.py lint issues * warning updated
crazy-cat
pushed a commit
to crazy-cat/incubator-mxnet
that referenced
this pull request
Oct 26, 2017
* fix segfault in kvstore_dist for row sparse by: moving copy before zpull, and handling the case of a nullptr * fix indent, and bring back references * Update kvstore.py Update docs * Update kvstore_dist.h Update comment * Update kvstore.py lint issues * warning updated
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes segfaults in kvstore_dist for row sparse, and fixes tests in dist_sync_kvstore
@eric-haibin-lin and I worked on this.
Checklist
Essentials
make lint)Changes
Comments
When pushes happen one after the other, without a pull in between, there is no ordering guarantee we provide now that the
WaitToWritewas removed in #8116 for performance reasons. The second push may start and finish before first push finishesPlease comment if we missed something, or if you have any suggestions, or if any clarifications are needed.