Reorganize examples #179
Reorganize examples #179
Conversation
mattdangerw
force-pushed
the
update-examples
branch
from
291638a to
7daf894
Compare
| FLAGS.short_seq_prob, | ||
| FLAGS.masked_lm_prob, | ||
| FLAGS.max_predictions_per_seq, | ||
| PREPROCESSING_CONFIG["max_seq_length"], |
There was a problem hiding this comment.
Seems we are mixing PREPROCESSING_CONFIG with flags. May I ask why we only move part of flags to config?
There was a problem hiding this comment.
Things related to input and output need to be flags I think. Input files, output files, etc.
Preprocessing hyperparameters I was moving to code, per our discussions as group. Maybe someday we will want to actually pull vocab files automatically from some fixed locations, but not for this PR.
That just leaves do_lowercase. I think that's better to keep as a flag as it is essentially a new entire variant of a model. bert_size + do_lowercase together decide which "bert" you are building, and then we pull the config for your that corresponds to that. I think that is the UX we want.
There was a problem hiding this comment.
I have the feeling that this definition does not have a clear boundary, so for me I feel that there are two places to change to get the right config. But I guess such UX discussion is hard to get a "best" solution, so I will just leave this to you to decide.
There was a problem hiding this comment.
Agreed the boundary is not clear, but the idea should be that more flags is always a bad thing, so we should avoid when possible without breaking workflows
examples/README.md
Outdated
| ├── __init__.py | ||
| ├── modelname_config.py | ||
| ├── modelname_model.py | ||
| ├── modelname_pretrain.py |
Yep, though I will need to check again after getting these changes in. |
mattdangerw
force-pushed
the
update-examples
branch
from
3c9d246 to
408a6c1
Compare
- Configuration in code not json - Toplevel README - Clearer script names - Better directory organization Fixes keras-team#177
mattdangerw
force-pushed
the
update-examples
branch
from
408a6c1 to
158793e
Compare
|
All comments addressed. |
Fixes #177