hybridize rnn and add model graph #13244

yifeim · 2018-11-13T04:22:53Z

Description

Add hybridize option and visualize the network in mxnet>=1.3.

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:https://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

Add hybridize option. Hybridize on RNN layers is newly available in mxnet>=1.3.
Add model export to mxboard. The end result is visualized via tensorboard and added to README.

Comments

The change is back-ward compatible. Especially, hybridize_optional would be an empty list in mxnet<1.3.

kalyc · 2018-11-15T16:52:28Z

Hi @yifeim could you please re-trigger CI to ensure that the build succeeds?

kalyc · 2018-11-15T22:18:16Z

@mxnet-label-bot add [pr-work-in-progress]

stu1130 · 2018-11-20T01:19:06Z

@mxnet-label-bot update [pr-awaiting-review, gluon]

vandanavk · 2018-11-26T22:14:36Z

example/gluon/word_language_model/README.md

@@ -28,7 +28,9 @@ python train.py --cuda --tied --nhid 650 --emsize 650 --epochs 40 --dropout 0.5
 ```
 python train.py --cuda --tied --nhid 1500 --emsize 1500 --epochs 60 --dropout 0.65 # Test ppl of 88.42
 ```
-
+```
+python train.py --export-only # hybridize and export model graph (requires mxboard)


Please add instruction to install mxboard

added link and made it outside of the main codes.

vandanavk · 2018-11-26T22:18:55Z

example/gluon/word_language_model/train.py

+parser.add_argument('--static-shape', action='store_true',
+ help='whether to use static-shape hybridize in mxnet>=1.3')
+parser.add_argument('--export-only', action='store_true',
+ help='export a symbol graph and exit')


Please mention the default values for all of these arguments

Added defaults as directed.

vandanavk · 2018-11-27T18:21:55Z

@roywei for review

yifeim · 2018-11-30T18:01:27Z

Is this related to my PR?
http:https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/incubator-mxnet/detail/PR-13244/5/pipeline

[INFO] Setting module np/mx/python random seeds, use MXNET_MODULE_SEED=1716360676 to reproduce.
test_gluon_model_zoo_gpu.test_inference ... [02:33:54] src/io/iter_image_recordio_2.cc:172: ImageRecordIOParser2: data/val-5k-256.rec, use 1 threads for decoding..
[02:33:57] src/operator/nn/mkldnn/mkldnn_base.cc:74: Allocate 32112640 bytes with malloc directly
[02:34:03] src/io/iter_image_recordio_2.cc:172: ImageRecordIOParser2: data/val-5k-256.rec, use 1 threads for decoding..
Sending interrupt signal to process
build.py: 2018-11-27 02:34:52,714 Signal 15 received, cleaning up...
build.py: 2018-11-27 02:34:52,714 Cleaning up containers
Terminated
build.py: 2018-11-27 02:34:57,229 ☠: stopped container 3c9b21b903de
build.py: 2018-11-27 02:34:57,290 🚽: removed container 3c9b21b903de
build.py: 2018-11-27 02:34:57,291 Cleaning up containers finished.
build.py: 2018-11-27 02:34:57,291 done. Exiting with error.
script returned exit code 143

roywei · 2018-12-11T04:44:24Z

@yifeim Thanks for the contribution. Is there any performance improvement after hybridize? Btw, you can trigger CI again by rebase or push an empty commit.

yifeim · 2018-12-11T04:48:42Z

I did not see any performance improvement after hybridization. My guess is that the computations are already elementary and asynchronous.

…

On Mon, Dec 10, 2018 at 11:45 PM Lai Wei ***@***.***> wrote: @yifeim <https://github.com/yifeim> Thanks for the contribution. Is there any performance improvement after hybridize? Btw, you can trigger CI again by rebase or push an empty commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13244 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIiQ6ldjvJn0DOETJVQSpXc3om97e4f2ks5u3zhVgaJpZM4Ya48l> .

sandeep-krishnamurthy

Thanks for your contributions. 2 questions.

Why should Hybridize even be an option? Why can't we just have it as default?
export only is confusing or mis-leading. export is referred to a model (network + params) not just network. And, why is this option required?

yifeim · 2018-12-29T19:20:33Z

1. As noted in both README and help strings, hybridizing an RNN is not supported in mxnet<=1.2. 2. Export is the most obvious way for me to dump a network, unless you recommend a different approach that I am not aware of?

…

On Sat, Dec 29, 2018 at 10:56 AM Sandeep Krishnamurthy < ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks for your contributions. 2 questions. 1. Why should Hybridize even be an option? Why can't we just have it as default? 2. export only is confusing or mis-leading. export is referred to a model (network + params) not just network. And, why is this option required? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13244 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIiQ6h7cUg0YOocWN_KGkUfSHxXLuo1mks5u97rHgaJpZM4Ya48l> .

sandeep-krishnamurthy · 2018-12-29T19:24:32Z

As noted in both README and help strings, hybridizing an RNN is not supported in mxnet<=1.2. 2. Export is the most obvious way for me to dump a network, unless you recommend a different approach that I am not aware of?
…
On Sat, Dec 29, 2018 at 10:56 AM Sandeep Krishnamurthy < @.> wrote: @.* commented on this pull request. Thanks for your contributions. 2 questions. 1. Why should Hybridize even be an option? Why can't we just have it as default? 2. export only is confusing or mis-leading. export is referred to a model (network + params) not just network. And, why is this option required? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13244 (review)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AIiQ6h7cUg0YOocWN_KGkUfSHxXLuo1mks5u97rHgaJpZM4Ya48l .

Yes, that makes sense. Thanks.
Export is fine. What I meant is 'export_only' does not convey that only network is exported.

yifeim · 2018-12-29T19:32:54Z

*@sandeep-krishnamurthy* If I understand correctly, perhaps I should also emphasize that there are models that are never hybridized for good reasons. Hybridize should always be “an option”, for two reasons: 1. For some algorithms, e.g., RNN models, hybridize offers no performance differences. 2. Hybridize turns a gluon model into a symbol model and hence it strips away all dynamic computation capabilities, such as if/else, for loop, and array indexing. While mxnet 1.3 offers some dynamic graph capabilities, they are still very limited. For example, if/else condition requires the outputs to always have the same dimensions, which is sometimes impractical.

…

On Sat, Dec 29, 2018 at 11:20 AM Yifei Ma ***@***.***> wrote: 1. As noted in both README and help strings, hybridizing an RNN is not supported in mxnet<=1.2. 2. Export is the most obvious way for me to dump a network, unless you recommend a different approach that I am not aware of? On Sat, Dec 29, 2018 at 10:56 AM Sandeep Krishnamurthy < ***@***.***> wrote: > ***@***.**** commented on this pull request. > > Thanks for your contributions. 2 questions. > > 1. Why should Hybridize even be an option? Why can't we just have it > as default? > 2. export only is confusing or mis-leading. export is referred to a > model (network + params) not just network. And, why is this option required? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#13244 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AIiQ6h7cUg0YOocWN_KGkUfSHxXLuo1mks5u97rHgaJpZM4Ya48l> > . >

yifeim · 2018-12-29T19:35:22Z

Ok I agree. On Sat, Dec 29, 2018 at 11:25 AM Sandeep Krishnamurthy < [email protected]> wrote:

…

1. As noted in both README and help strings, hybridizing an RNN is not supported in mxnet<=1.2. 2. Export is the most obvious way for me to dump a network, unless you recommend a different approach that I am not aware of? … <#m_-5660027600346315218_> On Sat, Dec 29, 2018 at 10:56 AM Sandeep Krishnamurthy < ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks for your contributions. 2 questions. 1. Why should Hybridize even be an option? Why can't we just have it as default? 2. export only is confusing or mis-leading. export is referred to a model (network + params) not just network. And, why is this option required? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13244 (review) <#13244 (review)>>, or mute the thread https://github.com/notifications/unsubscribe-auth/AIiQ6h7cUg0YOocWN_KGkUfSHxXLuo1mks5u97rHgaJpZM4Ya48l . 1. Yes, that makes sense. Thanks. 2. Export is fine. What I meant is 'export_only' does not convey that only network is exported. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13244 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIiQ6gakEaMe6qR1giiiMTDi0ZVwA_qXks5u98GOgaJpZM4Ya48l> .

yifeim · 2018-12-29T19:38:22Z

Can you make the changes? Thanks!

…

On Sat, Dec 29, 2018 at 11:35 AM Yifei Ma ***@***.***> wrote: Ok I agree. On Sat, Dec 29, 2018 at 11:25 AM Sandeep Krishnamurthy < ***@***.***> wrote: > > 1. As noted in both README and help strings, hybridizing an RNN is > not supported in mxnet<=1.2. 2. Export is the most obvious way for me to > dump a network, unless you recommend a different approach that I am not > aware of? > … <#m_2218069160341530556_m_-5660027600346315218_> > On Sat, Dec 29, 2018 at 10:56 AM Sandeep Krishnamurthy < ***@***.***> > wrote: ***@***.**** commented on this pull request. Thanks for your > contributions. 2 questions. 1. Why should Hybridize even be an option? Why > can't we just have it as default? 2. export only is confusing or > mis-leading. export is referred to a model (network + params) not just > network. And, why is this option required? — You are receiving this because > you were mentioned. Reply to this email directly, view it on GitHub <#13244 > (review) > <#13244 (review)>>, > or mute the thread > https://github.com/notifications/unsubscribe-auth/AIiQ6h7cUg0YOocWN_KGkUfSHxXLuo1mks5u97rHgaJpZM4Ya48l > . > > > 1. Yes, that makes sense. Thanks. > 2. Export is fine. What I meant is 'export_only' does not convey that > only network is exported. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#13244 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AIiQ6gakEaMe6qR1giiiMTDi0ZVwA_qXks5u98GOgaJpZM4Ya48l> > . >

anirudhacharya · 2019-01-11T22:13:50Z

@sandeep-krishnamurthy can you please review/merge this PR?

yifeim · 2019-01-24T02:30:13Z

@sandeep-krishnamurthy Thanks for the reviews so far. I agree with you but I am very confused what is exactly needed here. Please let me know how I can help. Thanks.

sandeep-krishnamurthy · 2019-02-06T00:38:19Z

@yifeim - Sorry for delayed response.
Rather than calling it "export_only", can we call it "export-model"? So it will be more clear for users?

Rest of changes LGTM. I was only concerned that "export_only" is not very intuitive word that gives the meaning of what that option does.

sandeep-krishnamurthy

Thank you!
LGTM

* hybridize rnn and add model graph * trigger CI * separate mxboard visualization * add options and she-bang * add defaults * trigger CI * rename export-model

hybridize rnn and add model graph

310215b

yifeim requested a review from szha as a code owner November 13, 2018 04:22

marcoabreu added the pr-work-in-progress PR is still work in progress label Nov 15, 2018

trigger CI

3e15195

marcoabreu added Gluon pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Nov 20, 2018

vandanavk reviewed Nov 26, 2018

View reviewed changes

Yifei Ma added 3 commits November 26, 2018 15:47

separate mxboard visualization

ae40838

add options and she-bang

bd82455

add defaults

6fb5d85

yifeim mentioned this pull request Nov 30, 2018

Feature request - einsum #10840

Closed

trigger CI

f804acc

sandeep-krishnamurthy reviewed Dec 29, 2018

View reviewed changes

rename export-model

4846791

sandeep-krishnamurthy approved these changes Feb 7, 2019

View reviewed changes

sandeep-krishnamurthy merged commit 506a25c into apache:master Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hybridize rnn and add model graph #13244

hybridize rnn and add model graph #13244

yifeim commented Nov 13, 2018

kalyc commented Nov 15, 2018

kalyc commented Nov 15, 2018

stu1130 commented Nov 20, 2018

vandanavk Nov 26, 2018

yifeim Nov 26, 2018

vandanavk Nov 26, 2018

yifeim Nov 26, 2018

vandanavk commented Nov 27, 2018

yifeim commented Nov 30, 2018

roywei commented Dec 11, 2018

yifeim commented Dec 11, 2018 via email

sandeep-krishnamurthy left a comment

yifeim commented Dec 29, 2018 via email

sandeep-krishnamurthy commented Dec 29, 2018

yifeim commented Dec 29, 2018 via email

yifeim commented Dec 29, 2018 via email

yifeim commented Dec 29, 2018 via email

anirudhacharya commented Jan 11, 2019

yifeim commented Jan 24, 2019

sandeep-krishnamurthy commented Feb 6, 2019 •

edited

Loading

sandeep-krishnamurthy left a comment

hybridize rnn and add model graph #13244

hybridize rnn and add model graph #13244

Conversation

yifeim commented Nov 13, 2018

Description

Checklist

Essentials

Changes

Comments

kalyc commented Nov 15, 2018

kalyc commented Nov 15, 2018

stu1130 commented Nov 20, 2018

vandanavk Nov 26, 2018

Choose a reason for hiding this comment

yifeim Nov 26, 2018

Choose a reason for hiding this comment

vandanavk Nov 26, 2018

Choose a reason for hiding this comment

yifeim Nov 26, 2018

Choose a reason for hiding this comment

vandanavk commented Nov 27, 2018

yifeim commented Nov 30, 2018

roywei commented Dec 11, 2018

yifeim commented Dec 11, 2018 via email

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

yifeim commented Dec 29, 2018 via email

sandeep-krishnamurthy commented Dec 29, 2018

yifeim commented Dec 29, 2018 via email

yifeim commented Dec 29, 2018 via email

yifeim commented Dec 29, 2018 via email

anirudhacharya commented Jan 11, 2019

yifeim commented Jan 24, 2019

sandeep-krishnamurthy commented Feb 6, 2019 • edited Loading

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Feb 6, 2019 •

edited

Loading