[MXNET-317] Add Data Parallel #10536

zhanghang1989 · 2018-04-12T23:09:01Z

Description

(Brief description on what this PR is about)

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

zhanghang1989 · 2018-04-13T00:15:22Z

Hi @eric-haibin-lin @piiswrong , DataParallel and Barrier (ParallelState) are included as discussed.
Barrier is renamed => ParallelState, as suggested by @szha

eric-haibin-lin · 2018-04-13T00:17:08Z

python/mxnet/gluon/contrib/parallel.py

+from ...ndarray import NDArray
+from ..utils import split_and_load
+
+__all__ = ['DataParallel', 'Barrier', 'parallel_apply', 'split_kwargs']


Maybe we don't want to include "Barrier" in __all__ and display it on the API doc. For sparse I'm considering adding & exposing a wrapper around Barrier so that users only need to pass in indices and get weights. Normally I don't think ppl need to see Barrier. SyncBN can still access it by parallel.Barrier

eric-haibin-lin · 2018-04-13T00:18:04Z

python/mxnet/gluon/contrib/parallel.py

+                output.wait_to_read()
+            with lock:
+                results[i] = output
+        except Exception as e:


What exception will occur here?

The model defined by the user may get some troubles when forwarding, this catch exception avoid printing errors in different threads. We will raise the exception when gathering the results.

eric-haibin-lin · 2018-04-13T00:35:27Z

python/mxnet/gluon/contrib/parallel.py

+        """
+        return self.out[idx]
+
+    def get(self):


I think Sparse can reuse the pull() interface

you mean removing get() function?

pengzhao-intel · 2018-04-13T01:23:23Z

@zhanghang1989 I am interested in this PR.
Could you elaborate what this PR is for?

zhanghang1989 · 2018-04-13T01:39:57Z

@pengzhao-intel This PR is mainly for Synchronized Cross GPU Batch Norm https://github.com/zhanghang1989/MXNet-Gluon-SyncBN

zhanghang1989 · 2018-05-17T03:50:18Z

I have finished editing this repo. Could you start reviewing? @eric-haibin-lin @piiswrong Please see the deployed docs http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-10536/12/api/python/gluon/contrib.html?highlight=dataparallel#mxnet.gluon.contrib.parallel.DataParallelModel

piiswrong · 2018-05-17T18:05:58Z

docs/api/python/gluon/contrib.md

+    :nosignatures:
+
+    DataParallelModel
+    DataParallelCriterion


I think we only need a DataParallel.

This make it easier for the users, because the situation is complicated for network with multiple outputs. We just handle the situation internally.

I think this is too many API's and prone to misuse.

Also we use the term Loss instead of Criterion

Will change the name :)

piiswrong · 2018-05-17T18:07:15Z

python/mxnet/gluon/contrib/parallel.py

+        The cross device operation is applying (e.g. AllReduce).
+    """
+    def __init__(self, counter, operation):
+        self.mutex = threading.Lock()


internal variables should start with _

piiswrong · 2018-05-17T18:07:58Z

python/mxnet/gluon/contrib/parallel.py

+        return len(self.list)
+
+    def __repr__(self):
+        return 'ParallelState'


piiswrong · 2018-05-17T18:08:13Z

python/mxnet/gluon/contrib/parallel.py

+        self.push_tasks = self.counter
+        self.reduce_tasks = self.counter
+
+    def __len__(self):


why is this needed?

We may want to get the number of devices outside. For example, we want to calculate the global mean by dividing the global sum by global number of elements (local number of elements * N devices)

piiswrong · 2018-05-17T18:09:09Z

python/mxnet/gluon/contrib/parallel.py

+                self.out = self.op(*self.list)
+                if isinstance(self.out, (list, tuple)):
+                    for xi in self.out:
+                        xi.wait_to_read()


why do you need to wait?

If we don't wait, the training of BN fails.

Is the async execution engine not able to handle this?

piiswrong · 2018-05-17T18:19:01Z

python/mxnet/gluon/contrib/parallel.py

+
+    Parameters
+    ----------
+    module : object


piiswrong · 2018-05-17T18:22:20Z

python/mxnet/gluon/contrib/parallel.py

+        return criterion_parallel_apply(self.module, inputs, targets, kwargs, self.sync)
+
+
+def split_load_kwargs(inputs, kwargs, ctx_list, batch_axis=0):


internal functions should start with _

piiswrong · 2018-05-17T18:22:51Z

python/mxnet/gluon/contrib/parallel.py

+
+    is_training = autograd.is_training()
+    is_recording = autograd.is_recording()
+    threads = [threading.Thread(target=_worker,


recreating threads at each forward is too slow

I mainly followed the PyTorch implementation. Could you show an example of improvement?

Use a thread pool at constructor and reuse it for each iteration?

If pytorch do it like this maybe its fine

piiswrong · 2018-05-17T18:23:23Z

python/mxnet/gluon/contrib/parallel.py

+                with autograd.record(is_training):
+                    output = tuple_map(module(*input, **kwargs))
+                    for out in output:
+                        out.wait_to_read()


The training fails without waiting. I think it may because of with autograd.record(is_training):

Why? What's the error? That shouldn't happen. Are you using most recent code? There was a recent fix for multithreading

I haven't tested recently, I will take a look.

Actually, people can use sync=False mode at most of the cases.

piiswrong · 2018-05-17T18:25:56Z

python/mxnet/gluon/contrib/parallel.py

+        self.op = operation
+        self._clear()
+
+    def push(self, x):


how about have a single wait function instead of push/pull.
Is push and pull ever called separately?

There was a case in 'Sparse' application, which only pull onetime from the master thread. We can change it to single function.

eric-haibin-lin · 2018-05-19T01:03:53Z

python/mxnet/gluon/contrib/parallel.py

+
+
+class Barrier(object):
+    """Shared NDArray for cross device operation.


Don't think "Shared NDArray" is accurate description

eric-haibin-lin · 2018-05-19T01:08:48Z

python/mxnet/gluon/contrib/parallel.py

+    """Shared NDArray for cross device operation.
+
+    A cross device operation that allows synchronized push and pull. It can be used in
+    Cross-gpu Sycnhronized Batch Normalization and Sparse Blocks.


Remove sparse block from doc for now. Will not be using this for the 1st version

piiswrong · 2018-05-21T20:12:03Z

python/mxnet/gluon/contrib/parallel.py

+        inputs, kwargs = _split_load_kwargs(inputs, kwargs, self.ctx_list)
+        assert(len(inputs) == len(self.ctx_list))
+        if len(self.ctx_list) == 1:
+            return tuple([tuple_map(self.module(*inputs[0], **kwargs[0]))])


(x,) makes a tuple

eric-haibin-lin · 2018-05-21T20:29:45Z

tests/python/unittest/test_contrib_parallel.py

+    net.collect_params().initialize()
+    criterion = gluon.loss.SoftmaxCELoss(axis=1)
+
+    def test_net_sync(net, criterion, sync, nDevices):


nDevices -> num_devices

eric-haibin-lin · 2018-05-21T20:32:31Z

tests/python/unittest/test_contrib_parallel.py

+        def forward(self, x):
+            idx = self.barrier.push(x)
+            y = self.barrier.pull(idx)
+            assert_allclose(y.asnumpy(), x.asnumpy(), rtol=1e-2, atol=1e-4)


should x and y be exactly the same? The tolerance seems large

eric-haibin-lin · 2018-05-21T20:38:46Z

python/mxnet/gluon/contrib/parallel.py

+class DataParallelModel(object):
+    """Data Parallelism
+
+    Hide the difference of single/multiple GPUs to the user.


Hide .. from ..

eric-haibin-lin · 2018-05-21T20:41:24Z

python/mxnet/gluon/contrib/parallel.py

+
+    Parameters
+    ----------
+    module : object


Is it better to call it block instead of module?

Block has a general meaning in Gluon. We can probably call it model, if you think that is better.

eric-haibin-lin · 2018-05-21T20:44:26Z

tests/python/unittest/test_contrib_parallel.py

+    # evaluation mode
+    for i in range(iters):
+        x = mx.random.uniform(shape=(8, 1, 28, 28))
+        y = net(x)


what specifically are you checking here?

mainly check the behavior of Barrier:
assert_allclose(y.asnumpy(), x.asnumpy(), rtol=1e-2, atol=1e-4)

eric-haibin-lin · 2018-05-21T20:51:28Z

python/mxnet/gluon/contrib/parallel.py

+                self.out = self.op(*self.list)
+                if isinstance(self.out, (list, tuple)):
+                    for xi in self.out:
+                        xi.wait_to_read()


Is the async execution engine not able to handle this?

…nto parallel

zhanghang1989 · 2018-07-02T22:04:40Z

Closing it due to no longer needed in SyncBN #11502

zhanghang1989 requested a review from szha as a code owner April 12, 2018 23:09

zhanghang1989 changed the title ~~Add Data Parallel~~ [MXNET-317] Add Data Parallel Apr 12, 2018

eric-haibin-lin reviewed Apr 13, 2018

View reviewed changes

eric-haibin-lin self-assigned this Apr 18, 2018

piiswrong mentioned this pull request Apr 20, 2018

8 gpu no speed up, delay caused by main thread #10627

Closed

zhanghang1989 requested review from marcoabreu, sergeykolychev, thirdwing and yzhliu as code owners May 16, 2018 18:27

current progress

61cceb6

zhanghang1989 force-pushed the parallel branch from c39c313 to 61cceb6 Compare May 16, 2018 19:04

zhanghang1989 added 4 commits May 16, 2018 13:14

data parallel label and criterion

cbe7169

unitest and docs

3632f68

tuplemap

6ae71c7

pylint

43df173

update docs

bf82df6

piiswrong suggested changes May 17, 2018

View reviewed changes

zhanghang1989 added 4 commits May 17, 2018 14:53

func name

30955da

ci

2bd5680

Merge remote-tracking branch 'upstream/master' into parallel

336edf1

Merge remote-tracking branch 'upstream/master' into parallel

e67d92d

eric-haibin-lin reviewed May 20, 2018

View reviewed changes

piiswrong reviewed May 21, 2018

View reviewed changes

eric-haibin-lin reviewed May 21, 2018

View reviewed changes

address some comments

3027a4e

zhanghang1989 added 3 commits May 23, 2018 16:33

Merge remote-tracking branch 'upstream/master' into parallel

f2e5e6e

Merge branch 'parallel' of github.com:zhanghang1989/incubator-mxnet i…

0992fdc

…nto parallel

rename to loss

10e04ed

zhanghang1989 closed this Jul 2, 2018

		return criterion_parallel_apply(self.module, inputs, targets, kwargs, self.sync)


		def split_load_kwargs(inputs, kwargs, ctx_list, batch_axis=0):



		class Barrier(object):
		"""Shared NDArray for cross device operation.

[MXNET-317] Add Data Parallel #10536

[MXNET-317] Add Data Parallel #10536

Conversation

zhanghang1989 commented Apr 12, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

zhanghang1989 commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pengzhao-intel commented Apr 13, 2018

zhanghang1989 commented Apr 13, 2018

zhanghang1989 commented May 17, 2018

Choose a reason for hiding this comment

zhanghang1989 May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piiswrong May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhanghang1989 commented Jul 2, 2018

zhanghang1989 commented Apr 12, 2018 •

edited

Loading

zhanghang1989 May 17, 2018 •

edited

Loading

piiswrong May 21, 2018 •

edited

Loading