`LSTMModule` in main or contrib?
ajratner opened this issue · 6 comments
Logically, seems cleaner to have it in contrib. However (1) this would require making a clean way to run contrib-specific tests, and (2) the LSTMModule is currently used in other tests in the main code, as an example of a more complex module, e.g. for the Tuner, determinism, etc. Let's discuss here!
My fundamental concern is that if a new commit breaks the LSTM module, we don't want to merge it in, meaning we want CI tests that use the LSTM (plus this lets us have regression tests for specific small datasets, e.g.). Thus the argument for it being part of the core rather than contrib. I don't think we care as much about this for other modules, but the LSTM is a fundamentally core module in a manner for text-based tasks.
One could make the same argument for a basic CNN -- maybe we can have a bare-bones LSTMModule or CNNModule in core in the same way we have IdentityModule and have users subclass those to make fancier things in contrib.
I like the idea of all modules being in contrib. Then our contract for what metal does on the EndModel side is very clear: we're not yet-another-module-zoo; we stitch together modules. In general, I think it'd be great if we did more importing of existing modules from other libraries and less rewriting them ourself. And throughout metal, all we'll ever require is that it's a valid nn.Module with the right input/output dims.
But more specifically:
re: (1) regardless of how we decide where the LSTMModule goes, I agree that we could have a "run all tests" script so that just core runs on Travis for every commit, but before any version update we run the full one or something.
re: (2) I don't think the Tuner or determinism test would lose much by using a Linear module instead? They're unit tests for a reason. And then our "all tests" script essentially becomes our integration tests checker.
I hear you, @jdunnmon, on wanting a base NLP and CV module in core as examples, and I guess if they were really minimal that might be ok, but I think I still lean toward the clearer contract: MeTaL core is for MTL supervision and stitching together modules, not for providing modules. Keeping all modules is contrib will keep us truer to that vision, I think.
I'm with @bhancock8 here on keeping modules in contrib, but @jdunnmon also completely agree with the first thing you said above (essentially why I opened this convo). However, I think the point is: if we have built the API / interface correctly, then making changes to the main trunk shouldn't break anything in the modules, and vice versa, as long as the interface between the two stays the same.
So, I think we should separate everything out, and then make sure we get this interface done right so proper separation is maintained
The issue that I had been running into and was using the LSTMModule to test for (rather than a linear input layer) was recursive/hierarchical construction of modules by the Tuner. That is, there was specifically a bug if you didn't re-create the embeddings when you recreated the LSTM when recreating the EndModel in the tuner...
However, I think we're good here now. And either way, this should be a property of the interface definition, not the modules or main trunk on either end
Now, who wants to actually do the work here :)