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

Simple similarity based solver #90

Closed pdasigi closed 8 years ago

matt-gardner commented 8 years ago

If you rebase this against the current master, you'll remove a bunch of (maybe all of) the remaining lint errors.

matt-gardner commented 8 years ago

This looks good. It would be even better if there were a test to make sure this will actually train, but feel free to merge without it, once the tests pass, if you want. It's up to you.

pdasigi commented 8 years ago

Update: Code passes TestMultipleChoiceSimilaritySolver::test_train_does_not_crash with Keras 1.0.8, but not with 1.1.0. Might be related to this issue: https://github.com/fchollet/keras/issues/2582

matt-gardner commented 8 years ago

Ok, I've looked into the error that you're seeing, and I think I figured out the problem. The issue is that the closure in the Lambda object has a reference to the solver object, and the solver object has a reference to the Lambda object through self.model.layers. So, when you try to copy the config object when saving it to json, you get into an infinite copy loop.

I looked at the keras code, and I don't think there's an easy way to fix this there. This is the code that is ultimately causing the problem, and there's no way around the issue if you want to save the closure correctly. We just have to change our use of Lambda. Specifically we need to do one of two things:

  1. Just don't ever use a Lambda layer in our solvers, because creating closures within solver objects will break the keras config code. Instead, write a simple layer and put it in the layers module.
  2. When you want to use a Lambda layer, put the function definition outside of a containing class. I was able to fix the problem by doing this. See the changes I made here. You can cherry-pick that commit and add it here, or make similar changes yourself, and it should pass tests.
pdasigi commented 8 years ago

That makes sense. Thanks a lot for looking into it, Matt!

matt-gardner commented 8 years ago

Looks great. Feel free to merge this when you want, after adding the comment I recommended.