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

[BUGFIX] [DOC] Update nlp.model.get_model documentation and get_model API #734

Merged
merged 3 commits into from
Jun 3, 2019

Conversation

leezu
Copy link
Contributor

@leezu leezu commented May 29, 2019

http://gluon-nlp.mxnet.io/api/modules/model.html currently does not
differentiate pre-defined models exposed by the get_model function and other
components. This PR is a quick attempt to improve the distinction.

Further, get_model used a hardcoded dataset=wikitext-2 argument. This is
likely by accident as the get_model function was initially mainly used for
language models, but now also provides access to other models such as Bert, for
which hardcoded dataset argument does not make sense.

For example, loading a Bert model with a prespecified Vocab currently requires doing

nlp.model.get_model('bert_12_768_12', dataset_name=None, vocab=vocabulary, ...)

instead of

nlp.model.get_model('bert_12_768_12', vocab=vocabulary, ...)

The latter version currently fails, as it will pass the "wikitext-2" as
dataset_name to the Bert model constructor.

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

  • Change nlp.model.get_model dataset argument default to None
  • Improve nlp.model docs

@leezu leezu requested a review from szha as a code owner May 29, 2019 12:01
@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

❗ No coverage uploaded for pull request head (fix_get_model@1c7d809). Click here to learn what that means.
The diff coverage is n/a.

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #734 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
- Coverage   90.56%   90.53%   -0.03%     
==========================================
  Files          65       65              
  Lines        6071     6075       +4     
==========================================
+ Hits         5498     5500       +2     
- Misses        573      575       +2
Impacted Files Coverage Δ
src/gluonnlp/model/__init__.py 96% <100%> (-0.16%) ⬇️
src/gluonnlp/data/corpora/google_billion_word.py 66.66% <0%> (-8.34%) ⬇️
src/gluonnlp/vocab/vocab.py 97.94% <0%> (ø) ⬆️
src/gluonnlp/model/utils.py 76.72% <0%> (ø) ⬆️
src/gluonnlp/data/utils.py 78.41% <0%> (+0.05%) ⬆️
...p/data/corpora/large_text_compression_benchmark.py 89.28% <0%> (+8.92%) ⬆️

@mli
Copy link
Member

mli commented May 29, 2019

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

@eric-haibin-lin
Copy link
Member

The Segmentation fault: 11 in CI seems unrelated, but I didn't notice similar failures recently ..

@szha szha added the release focus Progress focus for release label May 31, 2019
@mli
Copy link
Member

mli commented Jun 3, 2019

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

@eric-haibin-lin eric-haibin-lin merged commit 5c9fdd1 into dmlc:master Jun 3, 2019
@leezu leezu deleted the fix_get_model branch June 3, 2019 16:53
paperplanet pushed a commit to paperplanet/gluon-nlp that referenced this pull request Jun 9, 2019
… API (dmlc#734)

* Improve gluonnlp.model docs

* Fix nlp.model.get_model API

* Update model.rst
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.

4 participants