Allennlp: TrackEpochCallback is not an EpochCallback

[mahnerak]

Is there any reason why TrackEpochCallback should not inherit from EpochCallback?
https://github.com/allenai/allennlp/blob/5b30658514a00e11000e648fec23be11a998bd92/allennlp/training/trainer.py#L179-L188

[epwalsh]

No, this was an oversight. Thanks for catching it. Would you like to make a PR to fix?

[mahnerak]

Sure! I'll make a PR.

[github-actions[bot]]

@epwalsh this is just a friendly ping to make sure you haven't forgotten about this issue 😜

[epwalsh]

Hey @mahnerak are you still planning on making a PR for this? If not I can do it.

[mahnerak]

Yes, I do. I was just preparing a couple of other notes on Callbacks and wanted to submit them as a single PR.

[mahnerak]

@epwalsh The end callbacks are called within the try block.
https://github.com/allenai/allennlp/blob/1ca478ab674dd412447b2f11c820ca5664737cc2/allennlp/training/trainer.py#L1111
However, I was expecting end callbacks to be called even in case of an error. Or, IDK, maybe there should be another way to inform the callback about the end of the training (so it can do a cleanup).

[mahnerak]

As there are no many third-party callbacks implemented for AllenNLP (yet) and the backwards-compatibility is not an issue right now, I think it's better to revisit the callback API.
For example, we may need on_start() callback instead of firing on_epoch(epoch=-1).

Also, I don't get why there should be separate BatchCallback, EndCallback, EpochCallback, and a metaclass TrainCallback to rule them all. I believe you had your reasons to implement it this way. But why not to have a single Callback class with __init__(self, serialization_dir, trainer), on_start(), on_batch_start(), on_batch_end(), on_epoch_start(), on_epoch_end(), on_end(), on_error(), on_after_backward(), etc...?
If someone wants to have a callback just for epochs, they can implement only a single method on_epoch_end().

I just feel the API of callbacks is the only non-intuitive abstraction in AllenNLP.
Pytorch-lightning has much straightforward callback interface.

[epwalsh]

Yea... this was kind of a half-baked feature to compromise with the folks (including me) who wanted to stick with the CallbackTrainer. So I'm all for making improvements, but we're committed to strictly adhering to Semantic Versioning, and so any breaking changes will have to go into our next major release: v2.0.

We are already working towards our v2.0 release though on a separate branch, so you could make a PR to that branch if you're interested. The branch is called vision (because the main focus of the 2.0 release is supporting vision + text models).

[mahnerak]

Okay, so I'll try to make a PR on non-breaking changes and after that will make major changes on vision branch.

[epwalsh]

Sounds great