(Rebased) Add ELMo to BCN #1308
Conversation
| def test_elmo_but_no_set_flags_throws_configuration_error(self): | ||
| initial_working_dir = os.getcwd() | ||
| # Change directory to module root. | ||
| os.chdir(self.MODULE_ROOT) |
There was a problem hiding this comment.
this is rather hacky, perhaps we should consider always changing dir to self.MODULE_ROOT before every test?
There was a problem hiding this comment.
Why do we need to change the directory at all?
There was a problem hiding this comment.
I don't understand why you need to change directory here at all?
There was a problem hiding this comment.
The check that throws the ConfigurationError in the test is in __init__, so it's run after the ELMo object is created. Model.from_params(self.vocab, params.get("model")) complains when building Elmo.from_params that the options file is not where it expects, since it's a relative path in the fixture.
There was a problem hiding this comment.
Can we just change the path so that we don't have to change directories?
There was a problem hiding this comment.
unfortunately we can't, since in general we can make no assumptions about where the tests are run from.
There was a problem hiding this comment.
Oh, right, yeah, I forgot about that. I noticed yesterday that the SEMPRE files were getting generated at allennlp/data/ instead of at data/, and I wondered why, and this explains it. Fair enough.
| "trainable": false | ||
| } | ||
| }, | ||
| # BCN configs that use ELMo should not have an elmo_token_embedder |
There was a problem hiding this comment.
Can you put this comment above the text_field_embedder and say why not, rather than just stating it. Maybe "The BCN model will consume the arrays generated by the ELMo token_indexer independently of the text_field_embedder, so we miss out the key for that here".
There was a problem hiding this comment.
good idea, thanks!
| def test_elmo_but_no_set_flags_throws_configuration_error(self): | ||
| initial_working_dir = os.getcwd() | ||
| # Change directory to module root. | ||
| os.chdir(self.MODULE_ROOT) |
There was a problem hiding this comment.
I don't understand why you need to change directory here at all?
* Add ELMo to BCN * Fix paths * Move fixtures to proper place * Change dir so model is properly created with fixture paths * Edit usage comment in training config * Fix lint
Continuation of #1253 , rebased after the pytorch 0.4 PR was merged.
Opening a new PR for this, since i think the discussion in #1253 is fairly useful.