Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

Conversation

@eric-haibin-lin
Copy link
Member

As reported in #8097 WaitToWrite blocks push until all gradients are computed, which was introduced in #7082. After discussing it again with @mli we decided to remove it and updated the document accordingly.

@solin319

@piiswrong
Copy link
Contributor

I thought this was already removed before we release 0.11?

@eric-haibin-lin
Copy link
Member Author

That one was WaitToRead if I recall correctly.

@piiswrong
Copy link
Contributor

why did you add this?

@piiswrong piiswrong merged commit 41c7690 into apache:master Oct 1, 2017
@rahul003
Copy link
Member

rahul003 commented Oct 9, 2017

Removing this from master is breaking the test dist_sync_kvstore.py
The simplest case of check_default_keys for small array of size 'shape' fails.
If I understand this right, accurate serialization of pushes might not matter for actual neural network training, so we removed this Wait. So we need to change the tests?

Traceback (most recent call last):
  File "dist_sync_kvstore.py", line 176, in <module>
    test_sync_push_pull()
  File "dist_sync_kvstore.py", line 169, in test_sync_push_pull
    check_default_keys(kv, my_rank, nworker)
  File "dist_sync_kvstore.py", line 67, in check_default_keys
    check_diff_to_scalar(val, num)
  File "dist_sync_kvstore.py", line 30, in check_diff_to_scalar
    assert(np.sum(np.abs((A - x).asnumpy())) == 0),(rank, A.asnumpy(), x)
AssertionError: (None, array([[ 21.,  21.,  21.],
       [ 21.,  21.,  21.]], dtype=float32), 37)

@eric-haibin-lin
Copy link
Member Author

We should update the test and document for push

@eric-haibin-lin eric-haibin-lin deleted the kvstore-wait branch October 18, 2017 17:30
crazy-cat pushed a commit to crazy-cat/incubator-mxnet that referenced this pull request Oct 26, 2017
* remove waittowrite

* update doc

* fix grammar
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants