Autobatch

[beckdaniel]

This pull request implement a new training regimen that leverages DyNet autobatching. It's very similar to SimpleTrainingRegimen but accumulates losses sequentially before computing the forward and backward passes. batch_size is expected to be fixed at 1 while update_every will encode the "actual batch size".

This request also add a few YAML files to benchmark standard batching vs. autobatching.

[msperber]

Thanks! I have some minor comments and think we can merge after those are addressed.

[msperber]

Maybe we can avoid code duplication by making this a subclass of SimpleTrainingRegimen instead of TrainingRegimen, and removing checkpoint_and_save and update?

[beckdaniel]

Done. There is a little hack to call backward at the right time though, with a comment. Not sure if it is the best solution.

[msperber]

It would improve clarity to put all experiments in one file with a defaults experiment, and then several experiments that only change the relevant points. But this is not a well-documented feature and it's also ok for me to just do this myself after merging this PR.

[beckdaniel]

Thanks! Started to work on the changes but might need a few more days. A few questions:

1. Should I do some profiling analysis before getting this merged? @neubig mentioned something like this in the issue tracker
2. batch_size in theory could be > 1 but that doesn't seem to make sense in this new regimen. Should I enforce it to be 1 through an assert? Less flexible, but would ensure users set the batch size through the right parameter (update_every)

[beckdaniel]

Hmmm, I'm bit of a newbie with Github reviews... Should I just click on "Resolve conversation" on the points I addressed? Do I ask for another review when I addressed all the requested changes?

[msperber]

Sure, but you can also just write a message when you're done and I'll take another look, either way is fine.

I think checking that the batch size is set through update_every and not through the batcher and potentially throwing a ValueError would be a good idea.

Regarding the profiling, that would also be interesting information! We could probably also merge without that, but if you're planning on doing some profiling we could wait for that. In this case, you can prepend [WIP] to the title of the PR to communicate that this is not ready to merge.

[beckdaniel]

Ok, I think it is ready for another review now. The only that should be missing from the previous review is to unify the YAML files into a single one: not sure exactly how to do that.

As for the profiling, I'm ok with this being merged without profiling since it might be useful as is. I can always do another pull request with an improved version later.

[msperber]

Sure, that sounds good! The code looks good to me, and I'll send a PR to simplify the config later.

[philip30]

Addressing #536