search

allenai / deep_qa

A deep NLP library, based on Keras / tf, focused on question answering (but useful for other NLP too)
Apache License 2.0
404 stars 133 forks source link

removed theano support per issue #311 #324

Closed morrme closed 7 years ago

morrme commented 7 years ago

Fixes: Issue #311

Notes:

In asreader_sciq_dataset.json , asreader_who_did_what.json gareader_sciq.json, and gareader_who_did_what.json there was a "preferred_backend" attribute set to "theano". I wasn't sure if you wanted it changed to a different value or if it was no longer needed at all, so I commented it out.

In matrix_attention.py, bag_of_words.py, knowledge_combiners.py and backend_py, there are comments that discuss modifications made due to Theano. I left those comments as they were, in case you plan to go back and refactor.

nelson-liu commented 7 years ago

thanks for your contribution!

In asreader_sciq_dataset.json , asreader_who_did_what.json gareader_sciq.json, and gareader_who_did_what.json there was a "preferred_backend" attribute set to "theano". I wasn't sure if you wanted it changed to a different value or if it was no longer needed at all, so I commented it out.

+1 for just removing it / removing the preferred_backend logic

morrme commented 7 years ago

While I am working on these:

I left the import backend as K statements there in case they were needed for anything else but Travis doesn't like unused imports. Should I get rid of them?

matt-gardner commented 7 years ago

Yes, please remove unused imports.

matt-gardner commented 7 years ago

Also, from Nelson's comment earlier: these are the lines he was talking about when he mentioned removing the preferred backend functionality:

https://github.com/allenai/deep_qa/blob/e098061dfb12fdff7fc8842926a518dba84d1470/deep_qa/training/trainer.py#L128-L130 https://github.com/allenai/deep_qa/blob/e098061dfb12fdff7fc8842926a518dba84d1470/deep_qa/training/trainer.py#L171-L174 https://github.com/allenai/deep_qa/blob/e098061dfb12fdff7fc8842926a518dba84d1470/deep_qa/training/trainer.py#L618-L635

morrme commented 7 years ago

All requested changes have been made except for the ones related to this comment

In the meantime, I will work on the latest Travis CI report.

morrme commented 7 years ago

Circle CI has a problem with knowledge_combiners.py, but that's not a file I have touched just yet. Does this have to do with the recent changes since my PR was open ?

There is also now a failing test on Travis, can anyone look into it? @nelson-liu @matt-gardner

We are close!

matt-gardner commented 7 years ago

@morrme, this looks great, thanks for your work on this. The issue with the envelope test is unrelated to what you were doing; I'll figure out it, push a fix, and merge this.

morrme commented 7 years ago

@matt-gardner OK, sounds good. Happy to help and I appreciate your support!

matt-gardner commented 7 years ago

Looks like the problem with K.cumsum was that the default axis for K.cumsum is 0, while our method had a default axis of -1. Pushing a fix for this soon.

matt-gardner commented 7 years ago

The issue in CircleCI looks like it's because of an inherited call docstring from keras.layers.layer.call. I'm guessing this is just the first instance that it runs into, and it crashes immediately, but it actually happens in lots of places. @nelson-liu, any ideas how to fix this? Is it because of the :show-inheritance: directive?

matt-gardner commented 7 years ago

Ideally, we should not crash on docstrings that we don't have control over. I'm not sure at all how to make this easy, though...

nelson-liu commented 7 years ago

Is it because of the :show-inheritance: directive?

reasonable guess, but im surprised that this didn't come up earlier...

morrme commented 7 years ago

@nelson-liu @matt-gardner well remember we just made the change to Keras 2.0.3 in this commit so....maybe related?

Also , there's a message here about updating this branch with the recent changes on master. Should I do that?

matt-gardner commented 7 years ago

Yeah, the reason we're just seeing this now is because of this line, which was added 22 days ago.

I'm looking into a fix; it'll probably be just always excluding the call method from our docs. This should be fine, because we put the documentation into the class docstring, anyway.

And yes, @morrme, in order to merge, we require that you first make sure the PR is up-to-date with master. So, yeah, click the "update branch" button.

matt-gardner commented 7 years ago

Alright, we're good to go! Merging now.

nelson-liu commented 7 years ago

great, thanks @morrme + @matt-gardner !