[NGRAPH] MXNet - nGraph initial integration #12502

mbrookhart · 2018-09-10T20:10:14Z

Description

Integrate MXNet with the Intel nGraph compiler using the Subgraph API feature. This PR provides integration for a number of models on the CPU. More models and backends will be coming in future PRs.

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

nGraph Integration through the Subgraph API
Better performance on a number of CPU models

Comments

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

eric-haibin-lin · 2018-09-11T20:32:40Z

@zheng-da

src/operator/subgraph/partition_graph.cc

kalyc · 2018-09-14T18:16:49Z

Thanks for your contribution @mbrookhart
@mxnet-label-bot[pr-awaiting-review]

eric-haibin-lin · 2018-09-23T21:59:56Z

What's the test coverage? Do you need any help setting up the ngraph build test on MXNet CI?

mbrookhart · 2018-09-24T13:23:22Z

Hi Eric,

We have a secondary test system working, but yeah, it would awesome to get some help setting up MXNet's CI, we haven't figured out exactly how that all works yet.

Matthew

mbrookhart · 2018-09-24T22:37:16Z

@eric-haibin-lin I think we have the CI system properly building with nGraph, starting to look at unit tests now. We see a few tests fail because the Subgraph API changes the number of nodes in the graph. According to @zheng-da, this is expected behavior. Would we expect the CI job to fail if we enabled those tests?

vandanavk · 2018-09-25T17:26:48Z

@marcoabreu for review and inputs

ashokei · 2018-09-26T02:51:32Z

@szha @anirudh2290 as code owner can you please review let us know feedback if any; CI and full unit-test are enabled.

Let us know anything we can do to expedite review/merge process.

include/mxnet/ndarray.h

src/c_api/c_api_symbolic.cc

src/operator/contrib/ngraph-inl.h

src/operator/contrib/ngraph.cc

src/operator/subgraph/partition_graph.cc

zheng-da · 2018-09-27T00:57:49Z

@mbrookhart @ashokei sorry for the late review. are you going to provide tests? it seems your code isn't tested at all.

mbrookhart · 2018-09-27T03:42:26Z

@zheng-da Thanks for the review! We added nGraph build and unit test jobs to Jenkins, they're running all of tests/python/unittest against nGraph through the subgraph API, aside from a few tests who's assumptions are broken by the Subgraph API.

ashokei · 2018-09-27T16:30:44Z

thanks @zheng-da for detailed review. Since this change just reuses subgraph API, we want to mainly test mxnet unit-tests through ngraph with MXNET_SUBGRAPH_BACKEND. This PR does that, and tests/python/unittest in CI goes through mxnet -> ngraph.

reminisce

Can you prepare an example showing how to use this feature in the folder example?

reminisce · 2018-09-27T16:52:48Z

NGRAPH_README.md

+After building MXNet with nGraph support, users can enable nGraph backend by setting `MXNET_SUBGRAPH_BACKEND="ngraph"`environmental variable. 
+
+Gluon support is experimental and may or may not yield good performance. Gluon-NGraph 
+integration can be enabled by setting the environmental variable `MXNET_NGRAPH_GLUON=1`


Why is an additional evn var needed for Gluon? How different is it from MXNET_SUBGRAPH_BACKEND?

we use MXNET_NGRAPH_GLUON env var for enabling/disabling mxnet imperative/gluon execution support for ngraph integration. This is not related(does not effect) to MXNET_SUBGRAPH_BACKEND.

NGRAPH_README.md

src/executor/graph_executor.cc

src/operator/contrib/ngraph-inl.h

mbrookhart · 2018-09-27T20:29:03Z

@reminisce Thanks for the review! What kind of examples would you like to see? After compiling with USE_NGRAPH=1, all Module-based examples running on cpu should transparently work with nGraph CPU unless the user selects a different subgraph backend with MXNET_SUBGRAPH_BACKEND. Future PRs with other backends will certainly include examples.

anirudhacharya · 2019-03-01T22:25:44Z

@zheng-da @szha ping for another round of reviews :)

eric-haibin-lin · 2019-03-02T00:35:44Z

@ashokei could you resolve conflicts? Thanks

ashokei · 2019-03-04T18:13:14Z

thanks @anirudhacharya .
@eric-haibin-lin we fixed conflicts. @zheng-da @szha .

concerns addressed for now

szha

What's the plan for the bridge code?

louisfeng · 2019-03-07T23:19:49Z

@szha Our bridge code can be treated as 3rd party submodule and is publicly open source: https://github.com/NervanaSystems/ngraph-mxnet-bridge

karan6181 · 2019-03-19T00:45:34Z

@mbrookhart @ashokei Thank you for your contribution! can you please address the review comments? Thank you!

eric-haibin-lin · 2019-03-22T05:04:17Z

@szha any other concerns?

piyushghai · 2019-04-09T00:23:05Z

@mbrookhart Pinging again. Can you rebase with master branch and address the review comments ?

louisfeng · 2019-04-11T18:37:45Z

@mbrookhart Pinging again. Can you rebase with master branch and address the review comments ?

We have followed up with @szha a few weeks ago on the slack channel and waiting for him to approve this PR.

Roshrini · 2019-04-17T15:58:55Z

@szha Can you take a look at this PR? @mbrookhart Can you please resolve conflicts?

roywei · 2019-04-29T16:18:14Z

@mxnet-label-bot add [pr-awaiting-response]

vandanavk · 2019-05-07T21:54:35Z

@szha is this PR good to go? @mbrookhart could you resolve the merge conflicts?

louisfeng · 2019-05-08T02:21:07Z

@szha is this PR good to go? @mbrookhart could you resolve the merge conflicts?

We have resolved the merge conflicts a few times previously. It probably makes sense to resolve the conflicts before the merge once the PR is approved. Otherwise we need to continuously fixing the conflicts.

karan6181 · 2019-05-21T20:38:07Z

@szha , @anirudh2290 Could you please review this PR again? @louisfeng waiting for the response. Thanks!

abhinavs95 · 2019-05-31T22:13:23Z

@szha Could you please take a look again? Thanks.

piyushghai · 2019-06-07T22:29:49Z

@mbrookhart Can you rebase with the latest master ?

@szha @anirudh2290 Bouncing off for review...

Roshrini · 2019-06-23T20:21:20Z

@szha @anirudh2290 Ping again for review

karan6181 · 2019-07-18T16:50:46Z

@mbrookhart Could you please resolve the merge conflicts and @szha , @anirudh2290 will review it once the PR is updated!! Thanks!

piyushghai · 2019-08-02T20:04:32Z

@mbrookhart Could you please resolve the merge conflicts and @szha , @anirudh2290 will review it once the PR is updated!! Thanks!

cjolivier01 · 2019-11-19T05:41:06Z

src/operator/subgraph/partition_graph.cc


+  const SubgraphPropertyPtr& subg_prop = g->GetAttr<SubgraphPropertyPtr>("subgraph_property");
+  nnvm::NodePtr n;
+  if (!subg_prop->NeedGraphAttrs()) {


does this cause any overhead when ngraph isn’t enabled?

mbrookhart requested review from anirudh2290 and szha as code owners September 10, 2018 20:10

ashokei force-pushed the ngraph_initial_upstream branch 2 times, most recently from 0da8f18 to 6aca854 Compare September 11, 2018 05:14

mbrookhart force-pushed the ngraph_initial_upstream branch from 6aca854 to 67f99d2 Compare September 11, 2018 22:44

reminisce reviewed Sep 12, 2018

View reviewed changes

src/operator/subgraph/partition_graph.cc Show resolved Hide resolved

mbrookhart force-pushed the ngraph_initial_upstream branch from 7ffdd6b to 61bcdd1 Compare September 12, 2018 16:14

marcoabreu added the pr-awaiting-review PR is waiting for code review label Sep 14, 2018

mbrookhart force-pushed the ngraph_initial_upstream branch from 61bcdd1 to 1210584 Compare September 17, 2018 18:58

mbrookhart force-pushed the ngraph_initial_upstream branch from 1210584 to 2ffc093 Compare September 24, 2018 16:22

mbrookhart requested a review from marcoabreu as a code owner September 24, 2018 19:31

mbrookhart force-pushed the ngraph_initial_upstream branch from 235e29c to b7e6743 Compare September 24, 2018 20:44

ashokei force-pushed the ngraph_initial_upstream branch 2 times, most recently from c6e2149 to 82670da Compare September 26, 2018 02:39

ashokei force-pushed the ngraph_initial_upstream branch from 80f0b5b to b01608c Compare September 26, 2018 05:50

mbrookhart force-pushed the ngraph_initial_upstream branch from b01608c to 304a4c9 Compare September 26, 2018 16:09

zheng-da reviewed Sep 27, 2018

View reviewed changes

mbrookhart force-pushed the ngraph_initial_upstream branch from 90d9cee to 01599b9 Compare September 27, 2018 16:14

reminisce reviewed Sep 27, 2018

View reviewed changes

monitor callback disabled

fb3853e

ashokei added 4 commits March 1, 2019 18:53

resolve merge conflict

08bb32d

fix mxnet::ShapeVector

48fcb27

fix license header

bf89281

fix flaky test

bb0ee91

zheng-da approved these changes Mar 6, 2019

View reviewed changes

Merge branch 'master' into ngraph_initial_upstream

9acec60

szha suggested changes Mar 7, 2019

View reviewed changes

marcoabreu added the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Apr 29, 2019

mbrookhart closed this Aug 7, 2019

cjolivier01 reviewed Nov 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NGRAPH] MXNet - nGraph initial integration #12502

[NGRAPH] MXNet - nGraph initial integration #12502

mbrookhart commented Sep 10, 2018 •

edited

Loading

eric-haibin-lin commented Sep 11, 2018

kalyc commented Sep 14, 2018

eric-haibin-lin commented Sep 23, 2018

mbrookhart commented Sep 24, 2018

mbrookhart commented Sep 24, 2018

vandanavk commented Sep 25, 2018

ashokei commented Sep 26, 2018

zheng-da commented Sep 27, 2018

mbrookhart commented Sep 27, 2018 •

edited

Loading

ashokei commented Sep 27, 2018

reminisce left a comment

reminisce Sep 27, 2018

ashokei Sep 27, 2018

mbrookhart commented Sep 27, 2018 •

edited

Loading

anirudhacharya commented Mar 1, 2019

eric-haibin-lin commented Mar 2, 2019

ashokei commented Mar 4, 2019

szha left a comment •

edited

Loading

louisfeng commented Mar 7, 2019

karan6181 commented Mar 19, 2019

eric-haibin-lin commented Mar 22, 2019

piyushghai commented Apr 9, 2019

louisfeng commented Apr 11, 2019 •

edited

Loading

Roshrini commented Apr 17, 2019

roywei commented Apr 29, 2019

vandanavk commented May 7, 2019

louisfeng commented May 8, 2019 •

edited

Loading

karan6181 commented May 21, 2019

abhinavs95 commented May 31, 2019

piyushghai commented Jun 7, 2019

Roshrini commented Jun 23, 2019

karan6181 commented Jul 18, 2019

piyushghai commented Aug 2, 2019

cjolivier01 Nov 19, 2019

[NGRAPH] MXNet - nGraph initial integration #12502

[NGRAPH] MXNet - nGraph initial integration #12502

Conversation

mbrookhart commented Sep 10, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

eric-haibin-lin commented Sep 11, 2018

kalyc commented Sep 14, 2018

eric-haibin-lin commented Sep 23, 2018

mbrookhart commented Sep 24, 2018

mbrookhart commented Sep 24, 2018

vandanavk commented Sep 25, 2018

ashokei commented Sep 26, 2018

zheng-da commented Sep 27, 2018

mbrookhart commented Sep 27, 2018 • edited Loading

ashokei commented Sep 27, 2018

reminisce left a comment

Choose a reason for hiding this comment

reminisce Sep 27, 2018

Choose a reason for hiding this comment

ashokei Sep 27, 2018

Choose a reason for hiding this comment

mbrookhart commented Sep 27, 2018 • edited Loading

anirudhacharya commented Mar 1, 2019

eric-haibin-lin commented Mar 2, 2019

ashokei commented Mar 4, 2019

szha left a comment • edited Loading

Choose a reason for hiding this comment

louisfeng commented Mar 7, 2019

karan6181 commented Mar 19, 2019

eric-haibin-lin commented Mar 22, 2019

piyushghai commented Apr 9, 2019

louisfeng commented Apr 11, 2019 • edited Loading

Roshrini commented Apr 17, 2019

roywei commented Apr 29, 2019

vandanavk commented May 7, 2019

louisfeng commented May 8, 2019 • edited Loading

karan6181 commented May 21, 2019

abhinavs95 commented May 31, 2019

piyushghai commented Jun 7, 2019

Roshrini commented Jun 23, 2019

karan6181 commented Jul 18, 2019

piyushghai commented Aug 2, 2019

cjolivier01 Nov 19, 2019

Choose a reason for hiding this comment

mbrookhart commented Sep 10, 2018 •

edited

Loading

mbrookhart commented Sep 27, 2018 •

edited

Loading

mbrookhart commented Sep 27, 2018 •

edited

Loading

szha left a comment •

edited

Loading

louisfeng commented Apr 11, 2019 •

edited

Loading

louisfeng commented May 8, 2019 •

edited

Loading