-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AllenNLP integration. #1086
Add AllenNLP integration. #1086
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1086 +/- ##
=======================================
Coverage 90.46% 90.46%
=======================================
Files 131 131
Lines 11331 11331
=======================================
Hits 10251 10251
Misses 1080 1080 Continue to review full report at Codecov.
|
I think this PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adopting the new Allennlp api!
Why does the format of test.jsonnet look different from that of example.jsonnet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some small comments.
examples/allennlp/classifier.jsonnet
Outdated
"numpy_seed": 42, | ||
"pytorch_seed": 42, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numpy_seed
and pytorch_seed
seem to be in the same nest-level, but they have different indent-levels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing out, I run jsonnetfmt
in d591f53.
optuna/integration/allennlp.py
Outdated
"""Train a model using allennlp.""" | ||
|
||
for package_name in self._include_package: | ||
allennlp.common.util.import_module_and_submodules(package_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think any test cases do not visit this line. Please add test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added tests in 6ac976b and found there is a breaking change in this feature.
import_module_and_submodule
will be introduced in the next release allennlp,
which is the renamed method of import_modules
in v0.9.0
.
optuna/integration/allennlp.py
Outdated
serialization_dir: str, | ||
metrics: str = "best_validation_accuracy", | ||
*, | ||
include_package: List[str] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a description of include_package
to the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 6ac976b.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for parsing jsonnet.
(FYI. @crcrpar @toshihikoyanase )
TrackerCallback = object | ||
|
||
|
||
@experimental("1.4.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made AllenNLPExecutor
experimental since the major release of AllenNLP
will come soon.
There are a lot of updates in their usages and we could face the necessity of changing the interface.
For more information, please see https://github.com/allenai/allennlp/releases/tag/v1.0-prerelease.
@toshihikoyanase I also would like to express my gratitude to @crcrpar for patient helps. |
Co-Authored-By: Masaki Kozuki <masaki.kozuki.2014@gmail.com>
🙀 Co-Authored-By: Masaki Kozuki <masaki.kozuki.2014@gmail.com>
Co-Authored-By: Masaki Kozuki <masaki.kozuki.2014@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your update. It basically looks good to me.
I added small comments about docstring. Please check them.
|
||
This feature is experimental since AllenNLP major release will come soon. | ||
The interface may change without prior notice to correspond to the update. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding the link to the example as can be seen in TensorFlow's integration?
See the example if you want to add a pruning hook to TensorFlow’s estimator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion.
Added in 1533f6a.
Co-Authored-By: Toshihiko Yanase <toshihiko.yanase@gmail.com>
…a into feature/allennlp-cli-example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for your great contribution!
This PR introduces AllenNLP integration to run HPO with AllenNLP configuration files.
Example
#1078