Add allennlp example.
#949
Conversation
Codecov Report
@@ Coverage Diff @@
## master #949 +/- ##
==========================================
- Coverage 90.22% 90.19% -0.04%
==========================================
Files 112 114 +2
Lines 9293 9506 +213
==========================================
+ Hits 8385 8574 +189
- Misses 908 932 +24
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for your great PR!
I have some small comments. In addition, I found this example required hours of computational time without GPUs. Is it possible to reduce the computational time to about ten minutes because we execute all examples in daily CI using VMs without GPUs.
examples/allennlp_simple.py
Outdated
| import allennlp.modules | ||
| import optuna | ||
| import torch | ||
| import uuid |
There was a problem hiding this comment.
Please separate standard library imports and third library imports?
(c.f., #514)
examples/allennlp_simple.py
Outdated
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| optuna.logging.set_verbosity(optuna.logging.WARNING) |
There was a problem hiding this comment.
Could you tell me why you suppress Optuna's log messages?
There was a problem hiding this comment.
I didn't have some specific intention. 🙇
(I made this PR based on example/chainer_simple.py, which have this setting)
I removed it in 037d12a.
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
examples/allennlp_simple.py
Outdated
|
|
||
| # Run tuning with small portion of data | ||
| # to reduce computational time. | ||
| # https://github.com/optuna/optuna/pull/949#pullrequestreview-364110499 |
There was a problem hiding this comment.
IMO, we can remove this link according to the past PRs. Maybe it would be helpful if we share code conventions like this in a design document.
examples/allennlp_simple.py
Outdated
| patience=3, | ||
| num_epochs=6, | ||
| cuda_device=DEVICE, | ||
| serialization_dir=f'/tmp/xx/{uuid.uuid1()}', |
There was a problem hiding this comment.
It would be difficult for users to find the model corresponding to the specific trial. This is partly because the directory names are generated based on uuid and partly because the directory is not displayed in log messages. What do you think if we remove it for simplicity? If you remove it, please delete import uuid too.
| serialization_dir=f'/tmp/xx/{uuid.uuid1()}', |
There was a problem hiding this comment.
Based on pytorch_lightning_simple.py, I revise my PR to use trial.number for creating a model serialization directory. e534b92
setup.py
Outdated
| @@ -73,6 +73,7 @@ def get_extras_require(): | |||
| 'sphinx_rtd_theme', | |||
| ], | |||
| 'example': [ | |||
| 'allennlp', | |||
There was a problem hiding this comment.
allennlp does not support Python 3.5. (c.f., https://pypi.org/project/allennlp/). Please exclude it from the installation targets if the Python version is 3.5.
In addition, please exclude examples/allennlp from the CI targets if the job is examples-python35.
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
There was a problem hiding this comment.
LGTM. Thank you for your contribution.
[Note]
allennlp will change the arguments of trainer. For example, train_dataset and validation_dataset will be replaced with dataloader.
examples/allennlp_simple.py
Outdated
| MODEL_DIR = os.path.join(DIR, 'result') | ||
|
|
||
| GLOVE_FILE_PATH = ( | ||
| 'https://s3-us-west-2.amazonaws.com/allennlp/datasets/glove/glove.6B.50d.txt.gz' |
There was a problem hiding this comment.
Sorry I'm not familiar with AllenNLP but is it the case that this AWS URL (and the ones below) is officially documented somewhere?
There was a problem hiding this comment.
This example is based on the allentune's official example and this AWS URL is used here.
|
@hvy Thank you for taking the time, I revised my PR! |
examples/allennlp_simple.py
Outdated
| model = create_model(vocab, trial) | ||
|
|
||
| if DEVICE > -1: | ||
| model.cuda(DEVICE) |
There was a problem hiding this comment.
Nit: model.to(torch.device(’cuda:{}’.format(DEVICE))).
|
Let me modify the title of this PR to match the format of our release notes. |
This PR adds an example using allennlp for defining and training a model.
(edited: this example is based on allentune's official example.)