Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

[Transformer] Skip dropout layer when rate=0 #597

Merged
merged 8 commits into from
Mar 4, 2019
Merged

[Transformer] Skip dropout layer when rate=0 #597

merged 8 commits into from
Mar 4, 2019

Conversation

TaoLv
Copy link
Member

@TaoLv TaoLv commented Feb 18, 2019 •
edited
Loading

Description

#596

Checklist

Essentials

  • PR's title starts with a category (e.g. [BUGFIX], [MODEL], [TUTORIAL], [FEATURE], [DOC], etc)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented

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

@TaoLv TaoLv requested a review from szha as a code owner February 18, 2019 02:37
@TaoLv TaoLv mentioned this pull request Feb 18, 2019
Closed
@TaoLv TaoLv changed the title Skip dropout layer when rate=0 [Transformer] Skip dropout layer when rate=0 Feb 18, 2019
@mli
Copy link
Member

mli commented Feb 18, 2019

Job PR-597/1 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-597/1/index.html

@codecov
Copy link

codecov bot commented Feb 18, 2019 •
edited
Loading

Codecov Report

Merging #597 into master will decrease coverage by 0.51%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #597      +/-   ##
=========================================
- Coverage   65.12%   64.6%   -0.52%     
=========================================
  Files         135     135              
  Lines       12397   12199     -198     
=========================================
- Hits         8073    7881     -192     
+ Misses       4324    4318       -6
Flag Coverage Δ
#PR597 64.6% <100%> (ø) ⬆️
#PR617 ?
#master ?
#notserial 44.19% <0%> (+0.37%) ⬆️
#py2 64.34% <100%> (-0.53%) ⬇️
#py3 64.48% <100%> (-0.52%) ⬇️
#serial 49.62% <100%> (-0.78%) ⬇️

eric-haibin-lin
eric-haibin-lin reviewed Feb 18, 2019
View reviewed changes
Copy link
Member

@eric-haibin-lin eric-haibin-lin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! One comment:

src/gluonnlp/model/transformer.py
@@ -106,7 +107,8 @@ def __init__(self, units=512, hidden_size=2048, dropout=0.0, use_residual=True,
weight_initializer=weight_initializer,
bias_initializer=bias_initializer,
prefix='ffn_2_')
self.dropout_layer = nn.Dropout(dropout)
Copy link
Member

Choose a reason for hiding this comment

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

Might as well change

gluon-nlp/src/gluonnlp/model/transformer.py

Line 207 in 972d866

self.dropout_layer = nn.Dropout(dropout)

gluon-nlp/src/gluonnlp/model/transformer.py

Line 339 in 972d866

self.dropout_layer = nn.Dropout(dropout)

gluon-nlp/src/gluonnlp/model/transformer.py

Line 719 in 972d866

self.dropout_layer = nn.Dropout(dropout)

and

gluon-nlp/src/gluonnlp/model/transformer.py

Line 854 in 972d866

self.dropout_layer = nn.Dropout(dropout)

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Will change them accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Would you mind also adding the case when dropout is set to 0 in the unit test? https://github.com/dmlc/gluon-nlp/blob/master/tests/unittest/test_models.py#L75

@szhengac
Copy link
Member

Looks ok to me.

@TaoLv
Copy link
Member Author

TaoLv commented Feb 23, 2019

Looks like the failure in CI is not related to the changes in this PR. Any idea about how to get it passed?

@szha
Copy link
Member

szha commented Feb 23, 2019

@TaoLv I triggered the CI again just now. I will make the linkcheck optional and instead let @mli send a report to the PR if anything is broken

eric-haibin-lin
eric-haibin-lin reviewed Feb 24, 2019
View reviewed changes
Copy link
Member

@eric-haibin-lin eric-haibin-lin left a comment

Choose a reason for hiding this comment

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

Looks good pending unit tests

src/gluonnlp/model/transformer.py
@@ -106,7 +107,8 @@ def __init__(self, units=512, hidden_size=2048, dropout=0.0, use_residual=True,
weight_initializer=weight_initializer,
bias_initializer=bias_initializer,
prefix='ffn_2_')
self.dropout_layer = nn.Dropout(dropout)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Would you mind also adding the case when dropout is set to 0 in the unit test? https://github.com/dmlc/gluon-nlp/blob/master/tests/unittest/test_models.py#L75

@TaoLv
Copy link
Member Author

TaoLv commented Feb 26, 2019

Sure, will change accordingly.

@mli
Copy link
Member

mli commented Mar 3, 2019

Job PR-597/5 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-597/5/index.html

@mli
Copy link
Member

mli commented Mar 3, 2019

Job PR-597/6 is complete.
Docs are uploaded to http://gluon-nlp-staging.s3-accelerate.dualstack.amazonaws.com/PR-597/6/index.html

@eric-haibin-lin eric-haibin-lin merged commit cad5fc2 into dmlc:master Mar 4, 2019
paperplanet pushed a commit to paperplanet/gluon-nlp that referenced this pull request Jun 9, 2019
@TaoLv @eric-haibin-lin
[Enhancement] Skip dropout layer in Transformer when rate=0 (dmlc#597)
b7eab09 
* skip droput layer when rate=0

* address comments

* fix complain of CI

* retrigger

* trigger CI

* add test for dropout=0
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@szha szha szha approved these changes

@eric-haibin-lin eric-haibin-lin eric-haibin-lin approved these changes

@szhengac szhengac Awaiting requested review from szhengac

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TaoLv @mli @szhengac @szha @eric-haibin-lin