[Script] Add inference function for BERT classification #639

TaoLv · 2019-03-20T06:04:28Z

Description

(Brief description on what this PR is about)

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 · 2019-03-20T06:05:21Z

@eric-haibin-lin @pengxin99

mli · 2019-03-20T07:10:16Z

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

codecov · 2019-03-20T07:10:26Z

Codecov Report

Merging #639 into master will decrease coverage by 0.16%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
- Coverage   63.63%   63.46%   -0.17%     
==========================================
  Files         141      141              
  Lines       12815    12849      +34     
==========================================
  Hits         8155     8155              
- Misses       4660     4694      +34

Flag	Coverage Δ
#PR637	`?`
#PR639	`63.46% <0%> (?)`
#master	`?`
#notserial	`42.32% <0%> (-0.12%)`	⬇️
#py2	`63.22% <0%> (-0.17%)`	⬇️
#py3	`63.35% <0%> (-0.17%)`	⬇️
#serial	`49.24% <0%> (-0.14%)`	⬇️

codecov · 2019-03-20T07:10:28Z

Codecov Report

Merging #639 into master will decrease coverage by 0.08%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #639      +/-   ##
==========================================
- Coverage   63.23%   63.14%   -0.09%     
==========================================
  Files         141      141              
  Lines       13013    13023      +10     
==========================================
- Hits         8229     8224       -5     
- Misses       4784     4799      +15

Flag	Coverage Δ
#PR639	`63.55% <33.33%> (-0.02%)`	⬇️
#PR655	`?`
#master	`63.74% <33.33%> (ø)`	⬆️
#notserial	`42.21% <0%> (-0.04%)`	⬇️
#py2	`62.9% <33.33%> (-0.09%)`	⬇️
#py3	`63.03% <33.33%> (-0.05%)`	⬇️
#serial	`48.75% <33.33%> (-0.08%)`	⬇️

TaoLv · 2019-03-21T14:39:47Z

Do I need to deal with the codecov complaints?

szha · 2019-03-21T18:47:34Z

@TaoLv you can ignore it for now. I guess some aggregation of the reports may not be correct so I will look into that.

eric-haibin-lin

Thanks! A few comments;

scripts/bert/finetune_classifier.py

vanewu · 2019-04-05T12:35:28Z

scripts/bert/finetune_classifier.py

+    toc = time.time()
+    total_num = dev_batch_size * len(dev_data)
+    logging.info('Time cost={:.2f} s throughput={:.2f} samples/s'.format(
+        toc - tic, total_num / (toc - tic)))


lazy logging

logging.info('Time cost=%.2f s throughput=%.2f samples/s', toc - tic, total_num / (toc - tic))

Other logs should also consider lazy mode.

Thank you for your review @kenjewu. Why do you think lazy logging is better here?

If use lazy construction of the string, the logging string is not actually constructed unless logging actually happens. This will be more efficient

Thanks @kenjewu , and doubt that does the code below need to do lazy logging?
logging.info('params saved in : {0}'.format(params_saved))

not really, but this would: logging.info('params saved in : {0}', params_saved)

Thank you @kenjewu, @szha, @pengxin99 . Now it's fixed.

…classification-inference

mli · 2019-04-10T16:13:52Z

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

mli · 2019-04-13T15:12:52Z

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

eric-haibin-lin · 2019-04-13T22:24:36Z

scripts/bert/finetune_classifier.py

+    '--only_inference',
+    action='store_true',
+    help='whether to do inference only on dev data. '
+    'If true, will load params from --model_parameters.')


Can you add a test in gluon-nlp/scripts/tests/test_bert.py?

Not sure how to do that. It needs a params file to do inference.

Ok. Let's revisit it later once we have some pre-trained model uploaded that can be used for inference

mli · 2019-04-14T12:23:51Z

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

TaoLv · 2019-04-14T15:04:19Z

Now inference log looks as following:

(gluonnlp-py3) [lvtao@mlt-skx134 bert]$ GLUE_DIR=glue_data python finetune_classifier.py --task_name MRPC --batch_size 32 --dev_batch_size 8 --optimizer adam --epochs 3 --seed 1 --lr 5e-5 --only_inference --model_parameters output_dir/model_bert_MRPC_2.params
INFO:root:Namespace(accumulate=None, batch_size=32, bert_dataset='book_corpus_wiki_en_uncased', bert_model='bert_12_768_12', dev_batch_size=8, epochs=3, epsilon=1e-06, gpu=False, log_interval=10, lr=5e-05, max_len=128, model_parameters='output_dir/model_bert_MRPC_2.params', only_inference=True, optimizer='adam', output_dir='./output_dir', pad=False, pretrained_bert_parameters=None, seed=1, task_name='MRPC', warmup_ratio=0.1)
INFO:root:loading model params from output_dir/model_bert_MRPC_2.params
<model architecture>
INFO:root:processing dataset...
INFO:root:Now we are doing BERT classification inference on cpu(0)!
INFO:root:[Batch 10/51] loss=0.3314, metrics:accuracy:0.8625,f1:0.7137
INFO:root:[Batch 20/51] loss=0.3637, metrics:accuracy:0.8625,f1:0.7133
INFO:root:[Batch 30/51] loss=0.4236, metrics:accuracy:0.8417,f1:0.7063
INFO:root:[Batch 40/51] loss=0.4259, metrics:accuracy:0.8406,f1:0.6917
INFO:root:[Batch 50/51] loss=0.4452, metrics:accuracy:0.8325,f1:0.6912
INFO:root:Time cost=41.30s, throughput=9.88samples/s

mli · 2019-04-14T16:19:08Z

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

szha · 2019-04-15T00:46:54Z

LGTM. Ping @kenjewu @eric-haibin-lin for another round of review.

vanewu · 2019-04-15T04:00:55Z

scripts/bert/finetune_classifier.py

@@ -325,8 +330,37 @@ def evaluate(dataloader_eval, metric):
    logging.info(metric_str, *metric_val)


+def log_train(batch_id, batch_num, metric, step_loss, log_interval, epoch_id, learning_rate):
+    metric_nm, metric_val = metric.get()


Although this is just a small auxiliary function, it is more standard if you can add a function comment.

In addition, log_train and log_inference currently appear to be slightly different only when constructing a log string. Is it better to integrate as a function or to keep it so separated now?

Thank you @kenjewu . Doc string is added for each function. I would like to keep the functions separated as is.

mli · 2019-04-17T13:59:25Z

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

eric-haibin-lin

Comments addressed

* add inference function for finetune_classifier * fix some comments * lazy logging * log message functions * fix lint * add doc string

add inference function for finetune_classifier

0da357e

TaoLv requested a review from szha as a code owner March 20, 2019 06:04

eric-haibin-lin reviewed Apr 1, 2019

View reviewed changes

scripts/bert/finetune_classifier.py Outdated Show resolved Hide resolved

scripts/bert/finetune_classifier.py Outdated Show resolved Hide resolved

scripts/bert/finetune_classifier.py Outdated Show resolved Hide resolved

vanewu reviewed Apr 5, 2019

View reviewed changes

TaoLv added 2 commits April 10, 2019 23:05

fix some comments

1058a52

Merge branch 'master' of https://github.com/dmlc/gluon-nlp into bert-…

e83cf51

…classification-inference

lazy logging

4848f13

eric-haibin-lin reviewed Apr 13, 2019

View reviewed changes

log message functions

5d5b4e7

fix lint

c067a2c

vanewu reviewed Apr 15, 2019

View reviewed changes

add doc string

cc47b9f

eric-haibin-lin approved these changes Apr 19, 2019

View reviewed changes

szha approved these changes Apr 19, 2019

View reviewed changes

eric-haibin-lin merged commit 698c10b into dmlc:master Apr 22, 2019

astonzhang pushed a commit that referenced this pull request May 10, 2019

[Script] Add inference function for BERT classification (#639)

f0b2267

* add inference function for finetune_classifier * fix some comments * lazy logging * log message functions * fix lint * add doc string

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Script] Add inference function for BERT classification #639

[Script] Add inference function for BERT classification #639

TaoLv commented Mar 20, 2019

TaoLv commented Mar 20, 2019

mli commented Mar 20, 2019

codecov bot commented Mar 20, 2019

codecov bot commented Mar 20, 2019 •

edited

TaoLv commented Mar 21, 2019

szha commented Mar 21, 2019

eric-haibin-lin left a comment

vanewu Apr 5, 2019

TaoLv Apr 10, 2019

vanewu Apr 12, 2019

pengxin99 Apr 12, 2019

szha Apr 12, 2019

TaoLv Apr 13, 2019

mli commented Apr 10, 2019

mli commented Apr 13, 2019

eric-haibin-lin Apr 13, 2019

TaoLv Apr 14, 2019

eric-haibin-lin Apr 19, 2019

mli commented Apr 14, 2019

TaoLv commented Apr 14, 2019

mli commented Apr 14, 2019

szha commented Apr 15, 2019

vanewu Apr 15, 2019

TaoLv Apr 17, 2019

mli commented Apr 17, 2019

eric-haibin-lin left a comment

[Script] Add inference function for BERT classification #639

[Script] Add inference function for BERT classification #639

Conversation

TaoLv commented Mar 20, 2019

Description

Checklist

Essentials

Changes

Comments

TaoLv commented Mar 20, 2019

mli commented Mar 20, 2019

codecov bot commented Mar 20, 2019

Codecov Report

codecov bot commented Mar 20, 2019 • edited

Codecov Report

TaoLv commented Mar 21, 2019

szha commented Mar 21, 2019

eric-haibin-lin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mli commented Apr 10, 2019

mli commented Apr 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mli commented Apr 14, 2019

TaoLv commented Apr 14, 2019

mli commented Apr 14, 2019

szha commented Apr 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mli commented Apr 17, 2019

eric-haibin-lin left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 20, 2019 •

edited