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

[MODEL][API] Change BaseTransformerEncoder to HybridBlock #845

Closed
wants to merge 19 commits into from

Conversation

TaoLv
Copy link
Member

@TaoLv TaoLv commented Jul 24, 2019

Description

This PR intends to change the BaseTransformerEncoder to HybridBlock so we can export it without padding to fixed sequence length.
#789

TODOs:

  • branch if arange_like is not available
  • change export.py under scripts/bert/export/

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 July 24, 2019 06:31
@codecov
Copy link

codecov bot commented Jul 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@de73ce8). Click here to learn what that means.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #845   +/-   ##
=========================================
  Coverage          ?   79.65%           
=========================================
  Files             ?       66           
  Lines             ?     6379           
  Branches          ?        0           
=========================================
  Hits              ?     5081           
  Misses            ?     1298           
  Partials          ?        0
Impacted Files Coverage Δ
src/gluonnlp/model/bert.py 64.84% <0%> (ø)
src/gluonnlp/model/transformer.py 81.36% <92.3%> (ø)

@codecov
Copy link

codecov bot commented Jul 24, 2019

Codecov Report

❗ No coverage uploaded for pull request head (hybrid-block@702197e). Click here to learn what that means.
The diff coverage is n/a.

@szha szha added the release focus Progress focus for release label Jul 31, 2019
@eric-haibin-lin eric-haibin-lin changed the title [WIP][MODEL] Change BaseTransformerEncoder to HybridBlock [MODEL] Change BaseTransformerEncoder to HybridBlock Aug 5, 2019
@eric-haibin-lin
Copy link
Member

We can limit this PR to hybridize transformer encoder only. I can take care of the hybridization of BERTModel and the export script in a separate PR.

@eric-haibin-lin eric-haibin-lin changed the title [MODEL] Change BaseTransformerEncoder to HybridBlock [MODEL][API] Change BaseTransformerEncoder to HybridBlock Aug 5, 2019
@TaoLv
Copy link
Member Author

TaoLv commented Aug 6, 2019

Thank you for working on this, @eric-haibin-lin . Sure, we can change the export script in another PR.

@eric-haibin-lin
Copy link
Member

Fixed by #877

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release focus Progress focus for release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants