-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to change the directory at all?
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 don't understand why you need to change directory here at all?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just change the path so that we don't have to change directories?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.