-
Notifications
You must be signed in to change notification settings - Fork 3
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
Migrated changes that apply to loading model and optimizer #54
Conversation
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, but
- you do have some linting errors that still need fixing
- I would like a confirmation that this has been tested
- this could benefit from setting up an explicit unit test
mammoth/models/model_saver.py
Outdated
@@ -1,5 +1,6 @@ | |||
import os | |||
from collections import deque | |||
from glob import glob | |||
from collections import deque, defaultdict |
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.
defaultdict is imported but not used..?
@@ -332,6 +350,9 @@ def build_task_specific_model( | |||
if uses_adapters(model_opts): | |||
logger.info('Creating adapters...') | |||
create_all_adapters(nmt_model, model_opts, task_queue_manager) | |||
if checkpoint: | |||
# TODO: plug in properly |
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.
this TODO probably worth creating another issue
The linting errors have been fixed and the feature has been tested! |
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 the hard work :)
this looks good to me
This allows to load model and optimizer from checkpoint.
This does not reload the data state.