Fix dist kvstore for trainer and flaky dist kvstore test #11633

eric-haibin-lin · 2018-07-10T19:18:10Z

Description

Previously there were no unit tests for trainer with dist kvstore. #11429 introduced a bug for dist kvstore in trainer, where it calls pull(ignore_sparse=False) which is NOT implemented for dist kvstore. This PR corrects the API call and adds unit test for it. Now ignore_sparse is set to False for non-distributed kvstore.

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

marcoabreu · 2018-07-10T19:23:56Z

Hello Haibin,

please take note that integrationtest_ubuntu_gpu_dist_kvstore is currently disabled due to this test throwing exceptions (which strangely don't cause the run to fail). This is tracked at #11441

rahul003

LGTM

ctcyang · 2018-07-10T21:07:14Z

I see an error for gluon_step, but the other two nightly tests pass. I ran ../../tools/launch.py -n 7 --launcher local python dist_sync_kvstore.py --type=gluon_step. The error log is attached dist_kvstore_error.txt.

ctcyang

The three nightly tests now pass. LGTM

eric-haibin-lin · 2018-07-11T03:35:12Z

@marcoabreu I've fixed and enabled the test.

marcoabreu · 2018-07-11T18:44:50Z

Thank you. Did you investigate why there were errors thrown in the log but the test has not been marked as failed?

marcoabreu · 2018-07-11T18:46:26Z

tests/nightly/dist_sync_kvstore.py

+
+def test_gluon_trainer_step():
+    def check_trainer_step():
+        ctx = mx.cpu(0)


is this on purpose? We run our KVstore tests on a GPU instance. if we don't require GPU, please downgrade to using a cpu instance.

marcoabreu · 2018-07-11T19:19:42Z

ci/docker/runtime_functions.sh

+    ../../tools/launch.py -n 7 --launcher local python dist_sync_kvstore.py --type=invalid
+    ../../tools/launch.py -n 7 --launcher local python dist_sync_kvstore.py --type=gluon_type
+    ../../tools/launch.py -n 7 --launcher local python dist_sync_kvstore.py --type=gluon_step
+    ../../tools/launch.py -n 7 --launcher local python dist_sync_kvstore.py --type=gluon_sparse_step


Sorry, I'm having a hard time to determine which tests require a gpu and which ones dont. Would it be possible to add some way to distinguish the tests (like adding cpu and gpu to the names, separating them into different files or adding an argument).

marcoabreu · 2018-07-11T19:19:54Z

Jenkinsfile

+        }
+      }
+    },
+    'dist-kvstore tests CPU': {


eric-haibin-lin · 2018-07-11T20:29:26Z

Re: "Did you investigate why there were errors thrown in the log but the test has not been marked as failed?"

Yes, i found init_kv() was called twice and throwing that exception. Now it is called only once.

marcoabreu · 2018-07-11T20:32:06Z

Sure, but I mean why the script did not terminate with an error code. The exceptions were logged but the script did not actually fail with an error. We only noticed the exception in the log when we looked at them manually

eric-haibin-lin · 2018-07-11T22:34:13Z

I thought @rahul003 fixed that? Could you confirm?

eric-haibin-lin · 2018-07-12T23:24:00Z

@rahul003 and I figured that the timeout was due to significant launch overhead of omp auto tuning. This is extremely slow when launching the test in local mode and all 15 processes are doing omp tuning, sharing the same instance. Creating kvstore takes 2 minutes when n=3.

marcoabreu · 2018-07-12T23:36:21Z

Awesome! Am I right in assuming that nobody would run this locally?

eric-haibin-lin · 2018-07-12T23:50:24Z

nobody except CI :)

marcoabreu · 2018-07-12T23:51:51Z

Okay excellent

rahul003

LGTM

rahul003 · 2018-07-13T00:46:20Z

tests/nightly/dist_sync_kvstore.py

        kv = set_optimizer(use_multiprecision=opt.multiprecision)
        test_sync_push_pull(opt.nrepeat)
-    # dont run non compressed tests after this as kvstore compression will be set here
+    # don't run non compressed tests after this as kvstore compression will be set here
    if opt.type == 'all' or opt.type == 'compressed':


Could you remove this value all now?

why remove it?

Because in the current script, it no longer represents all tests.

* fix dist kvstore trainer * fix test setup * enable tests on CI * update move some test to cpu * dont use nvdia-docker * rename option * trigger test * reduce workload to avvoid time out * disable operator tuning to reduce launch overhead * update test types

fix dist kvstore trainer

54eeaaf

eric-haibin-lin requested a review from szha as a code owner July 10, 2018 19:18

eric-haibin-lin changed the title ~~[WIP] Fix dist kvstore for trainer~~ Fix dist kvstore for trainer Jul 10, 2018

rahul003 approved these changes Jul 10, 2018

View reviewed changes

fix test setup

15be79c

eric-haibin-lin requested a review from anirudh2290 as a code owner July 10, 2018 23:07

ctcyang approved these changes Jul 10, 2018

View reviewed changes

enable tests on CI

7a22ea3

eric-haibin-lin requested a review from marcoabreu as a code owner July 11, 2018 01:20

eric-haibin-lin changed the title ~~Fix dist kvstore for trainer~~ Fix dist kvstore for trainer and flaky dist kvstore test Jul 11, 2018

marcoabreu reviewed Jul 11, 2018

View reviewed changes

update move some test to cpu

adc933d

marcoabreu reviewed Jul 11, 2018

View reviewed changes

Jenkinsfile

}

}

},

'dist-kvstore tests CPU': {

Copy link

Contributor

marcoabreu Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

dont use nvdia-docker

9dda024

rename option

64cd900

eric-haibin-lin added 5 commits July 12, 2018 06:00

trigger test

103e6e1

Merge branch 'master' into fix-dist-kv

b735964

reduce workload to avvoid time out

ece12ea

disable operator tuning to reduce launch overhead

ce8a3e9

merge

c6e1f3b

marcoabreu approved these changes Jul 12, 2018

View reviewed changes

rahul003 approved these changes Jul 13, 2018

View reviewed changes

rahul003 reviewed Jul 13, 2018

View reviewed changes

update test types

4649dcc

eric-haibin-lin merged commit 11cd73f into apache:master Jul 13, 2018

szha mentioned this pull request Jul 16, 2018

Some Python 3 fixes in ./example #11671

Merged

7 tasks

eric-haibin-lin deleted the fix-dist-kv branch November 12, 2018 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dist kvstore for trainer and flaky dist kvstore test #11633

Fix dist kvstore for trainer and flaky dist kvstore test #11633

eric-haibin-lin commented Jul 10, 2018 •

edited

Loading

marcoabreu commented Jul 10, 2018

rahul003 left a comment

ctcyang commented Jul 10, 2018 •

edited

Loading

ctcyang left a comment

eric-haibin-lin commented Jul 11, 2018

marcoabreu commented Jul 11, 2018

marcoabreu Jul 11, 2018

marcoabreu Jul 11, 2018

marcoabreu Jul 11, 2018

eric-haibin-lin commented Jul 11, 2018

marcoabreu commented Jul 11, 2018 •

edited

Loading

eric-haibin-lin commented Jul 11, 2018

eric-haibin-lin commented Jul 12, 2018

marcoabreu commented Jul 12, 2018

eric-haibin-lin commented Jul 12, 2018

marcoabreu commented Jul 12, 2018

rahul003 left a comment

rahul003 Jul 13, 2018 •

edited

Loading

eric-haibin-lin Jul 13, 2018

rahul003 Jul 13, 2018

Fix dist kvstore for trainer and flaky dist kvstore test #11633

Fix dist kvstore for trainer and flaky dist kvstore test #11633

Conversation

eric-haibin-lin commented Jul 10, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

marcoabreu commented Jul 10, 2018

rahul003 left a comment

Choose a reason for hiding this comment

ctcyang commented Jul 10, 2018 • edited Loading

ctcyang left a comment

Choose a reason for hiding this comment

eric-haibin-lin commented Jul 11, 2018

marcoabreu commented Jul 11, 2018

marcoabreu Jul 11, 2018

Choose a reason for hiding this comment

marcoabreu Jul 11, 2018

Choose a reason for hiding this comment

marcoabreu Jul 11, 2018

Choose a reason for hiding this comment

eric-haibin-lin commented Jul 11, 2018

marcoabreu commented Jul 11, 2018 • edited Loading

eric-haibin-lin commented Jul 11, 2018

eric-haibin-lin commented Jul 12, 2018

marcoabreu commented Jul 12, 2018

eric-haibin-lin commented Jul 12, 2018

marcoabreu commented Jul 12, 2018

rahul003 left a comment

Choose a reason for hiding this comment

rahul003 Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

eric-haibin-lin Jul 13, 2018

Choose a reason for hiding this comment

rahul003 Jul 13, 2018

Choose a reason for hiding this comment

eric-haibin-lin commented Jul 10, 2018 •

edited

Loading

ctcyang commented Jul 10, 2018 •

edited

Loading

marcoabreu commented Jul 11, 2018 •

edited

Loading

rahul003 Jul 13, 2018 •

edited

Loading