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 132 forks source link

Remove theano support #311

Closed matt-gardner closed 7 years ago

matt-gardner commented 7 years ago

This is really just dropping the theano condition from TravisCI, and removing the @requires_theano / @requires_tensorflow annotations. We're moving towards just using tensorflow, which will allow for a lot more flexibility in building models.

morrme commented 7 years ago

Is this one open to new contributors?

matt-gardner commented 7 years ago

Feel free to grab any unassigned issue and say you're working on it, and submit a PR. We're happy to have help.

morrme commented 7 years ago

I would like to give this one a try.

Affected files I've found:

https://github.com/allenai/deep_qa/blob/master/setup.py https://github.com/allenai/deep_qa/blob/master/requirements.txt https://github.com/allenai/deep_qa/blob/master/.travis.yml https://github.com/allenai/deep_qa/blob/master/scripts/py.test.sh

Are there others I may have missed?

nelson-liu commented 7 years ago

@morrme almost, you also have to get rid of the test markers in here (https://github.com/allenai/deep_qa/blob/master/tests/common/test_markers.py), as well as wherever they're used in the tests.

In addition, any thoughts on forcing tensorflow (i.e. raising an error if the backend is Theano)?

matt-gardner commented 7 years ago

@nelson-liu, I probably wouldn't force tensorflow, and I'd avoid using it anywhere outside of layers and models, or configurable parts of the trainer code, so that we can still be as general as possible, and minimize the required work if we want to, e.g., support pytorch some day. (I really doubt that this will ever happen, but it's still a good idea to keep the code modular, and decouple dependencies as much as possible.)

Also, @morrme, there are several places in non-test code where we make calls to K.backend(), to check whether we're using tensorflow before doing an import, or to give a warning or crash if we're using an unsupported backend in a particular place. Probably all of those checks can go away, and we can just assume that it's always tensorflow.

matt-gardner commented 7 years ago

Though, actually, the more I think about it, the more I agree with Nelson that it's probably a good idea to have a warning that things will likely crash if you're using theano. That would be something like an additional logger.warning here.

matt-gardner commented 7 years ago

@morrme, I need to up the priority on this issue, because it's now blocking merging #320, as well as some other P0 issues. Do you have any preliminary work on this that you can open a PR with? If you've started, I can help finish it, so we can get it merged.

morrme commented 7 years ago

@matt-gardner i am going to push my changes up tonight.

nelson-liu commented 7 years ago

Perhaps we should close this now?