Allennlp: Have a single `TrainerCallback` that can handle both `BatchCallback` and `EpochCallback`.

[dirkgr]

Also, add another call at the end of the whole training run.

This should make it easier to hang on to state inside the callback.

See the discussion at the end of this issue: https://github.com/allenai/allennlp/pull/3970

[viking-sudo-rm]

I can take this on next week. If I understand correctly, the intended change is that a new Callback class should have three separate methods for each event?

I don't quite understand the suggestion in #4674 that callbacks should be a context manager.

[mahnerak]

@viking-sudo-rm Maybe I was not clear.
Callbacks should have a method close() that is guaranteed to be called, so resources (say, we implement a logger with Callback) can be properly closed.
This may seem to be equivalent of having something like on_training_end(). In most cases it is true, but if the training is failed because of an error, we may want to close the files, but not to log the run as completed experiment (say, we have a callback which logs the best metrics of each successful run), so it's better to have a separate close() method.

[viking-sudo-rm]

I see, thanks for the clarification! That makes sense.

I've already started on a PR that doesn't include those changes, so I don't think I will address them now, but a follow-up PR should.

[dirkgr]

This was done with #4708.

[jacobdanovitch]

Quick question about this: I noticed that trainer_callbacks is a parameter in GradientDescentTrainer.__init__, but not in GradientDescentTrainer.from_partial_objects. This makes it impossible to use our own TrainerCallback objects from config files - is this by design?

An example use case for this would be for implementing a logger for Wandb/mlflow/etc. Most of them share a central run kind of object or need to be initialized, which is tricky to coordinate with separate callbacks. It's only an extra two or three lines in GradientDescentTrainer.from_partial_objects as well, so wasn't sure if it was left out intentionally.

[epwalsh]

@jacobdanovitch this was definitely an oversight. Would you be interested in making a PR to add that parameter to from_partial_objects?

[jacobdanovitch]

@jacobdanovitch this was definitely an oversight. Would you be interested in making a PR to add that parameter to from_partial_objects?

Absolutely, I'll get started.