Refactor train loop for easier customization

[BenjaminBossan]

Small refactoring of some training/evaluation methods

The main change is to "push down" the unpacking of the batch
data (using Xi, yi = unpack_data(batch)) from run_single_epoch to
functions lower down the chain. The reason for this is that if we put
this unpacking rather "high up" the chain, we force the output of
dataset to be a tuple of 2 elements. If a user wants to change this,
they either have to modify their dataset to always return 2
elements (which can be annoying) or they would need to override
run_single_epoch, but we don't want to encourage this, as this
method performs a lot of book keeping.

With the new changes, small methods further down the chain are now
responsible for dealing with the data, e.g. train_step_single and
validation_step. This affords more flexibility overall, and a user
can easily override these methods without having to deal with book
keeping.

Obviously, this change results in some API changes, so some existing
code might break as a consequence (most notably code that relies on
on_batch_begin or on_batch_end now simply get a batch which
they need to unpack, instead of separate X and y).

Another "victim" of this change is the checking for whether y is a
placeholder and then replacing it with None. Tbh. I found this to be
an ugly solution anyway. Now it can happen that when a user uses the
skorch Dataset, they will receive a 0s tensor for y when they set y to
None. I think this is an acceptable loss overall.

To illustrate the gains of the change, I added a test,
test_customize_net_with_custom_dataset_that_returns_3_values, which
shows what is now possible when it comes to customizing skorch. With
the old state, achieving the same would have been more cumbersome.

Finally, I added a section to the docs called "Customization". The
intent is to help the user decide how best to customize NeuralNet et
al. It explains what methods best to override for what purpose and
what methods to best leave untouched. In the future, we could expand
this section to cover more aspects.

Additional minor changes

• Clean ups to satisfy pylint
• Add more to .gitignore
• Remove duplication from .coveragerc

[BenjaminBossan]

Forgot to mention that I moved the notification for on_grad_computed from train_step_single to train_step. The reasoning for this was that a user is more likely to modify train_step_single, so if we move this book keeping out, users don't need to remember it.

And even if a user changes train_step, they most likely won't call train_step_single anymore, so if the notification was inside train_step_single, they still need to remember to include it into train_step (actually, remembering this might be easier now since it's right there in the code of train_step).

[thomasjpfan]

A quick look at this PR.

Overall, I like pushing the unpack_data down a level. My biggest concern is the backward compatibility. I do not think that batch level callbacks are too common, so it is most likely a non issue.

[thomasjpfan]

I suspect that changing the signature of callbacks would break's users code the most.

[githubnemo]

I suspect that changing the signature of callbacks would break's users code the most.

Agreed. Not every user might follow the changelog closely, we could wrap the notify call with an exception handler and in case of on_batch_begin we could raise the original exception in addition to a note informing the user about the change in signature and how to fix it. This could stay there for one release.

[BenjaminBossan]

I took a stab at this but it turns out not to be so simple. The reason is that we always have **kwargs in the signature, so even if someone writes a wrong method like def on_batch_begin(net, X=None, y=None, **kwargs), it would not immediately lead to an error. The error occurs only when they do something with X or y, because these will be None instead of the expected values. But this something can be anything.

In my attempt, I just caught any kind of Exception and raised a TypeError from it that pointed at the possible error source. However, this broke other tests, which checked for specific errors (for instance, using scoring with a non existing function name). We don't want that to happen.

The problem is thus that we don't know what error will occur but we also don't want to catch all kinds of errors.

We could theoretically inspect the arguments inside the notify-wrapper. But there I would be afraid that the performance penalty could be too big. Furthermore, we want to allow the users to be very liberal in how they call their on_* methods (e.g. only passing the arguments that they need, which might not even include batch), so it's not possible to do a strict check.

This leaves me with no idea how to implement the suggestion. Do you have any ideas?

[ottonemo]

OK this is fair, maybe we can issue a warning on the first call of notify('on_batch_begin') for callbacks that aren't ours that indicates that things might have changed? We can deprecate this warning in the next or a following release and you could always filter the warning if you are getting annoyed by it.

[BenjaminBossan]

So basically on the very first call to notify, you'd like to go through all callbacks and check whether they are an instance of any built-in callback, and if at least one of them isn't, issue a warning? I'm not sure if this is not overkill, given that the warning will mostly be a false alarm.

[ottonemo]

Oh, no, more like if not cb.__module__.startswith('skorch'): issueWarning().

[thomasjpfan]

In this case, I think it returns the loss as a float.

[BenjaminBossan]

I wasn't happy with the docstring and adjusted it. Mentioned that loss should be a float.

[BenjaminBossan]

My biggest concern is the backward compatibility. I do not think that batch level callbacks are too common, so it is most likely a non issue.

Yes, but I think we're still at a stage where we can afford to make such a breaking change. Honestly, I was surprised how little skorch code needed to be changed, which makes me believe, as you say, that the two methods are not used all too frequently.

It would probably help as a "transition guide" to mention:

# change from this:
def on_batch_begin(self, net, X, y, ...):
    ...

# to this:
def on_batch_begin(self, net, batch, ...):
    X, y = batch
    ...

# same for on_batch_end

Not sure where we could put this. But overall, there is really not much to it, so the gain outweighs the inconvenience IMO.

[BenjaminBossan]

@ottonemo are you fine with the changes?

[thomasjpfan]

I am in favor.

Not sure where we could put this. But overall, there is really not much to it, so the gain outweighs the inconvenience IMO.

In the FAQ, with a link in the change log?

[BenjaminBossan]

In the FAQ, with a link in the change log?

Great idea, I added a migration guide section to the FAQ.

[githubnemo]

This is probably a line that can be classified as bookkeeping. The intention of having it in a place where the user might implement their own training code was that, in case the user has two or more models (or set of parameters) the process of communicating gradient changes to callbacks is kept transparent.

With this change we make that a bit less transparent (at the benefit of recognizability - this looks way more standard now). Does this change therefore introduce the assumption that a user who adds additional modules (or parameter sets that are updated independently) will also override train_step_single?

[BenjaminBossan]

in case the user has two or more models (or set of parameters) the process of communicating gradient changes to callbacks is kept transparent.

What would be a use case where the user wants to call on_grad_computed explicitly here? E.g. when they only want to clip gradients on a subset of parameters, so that they would only pass self.module_.SUBMODULE.named_parameters()? If so, yes, that would require them to override train_step_single, which is not nice. I would argue, however, that this filtering should happen on the other side (i.e. in the callback), not here.

this looks way more standard now

That was the idea: Make this look as much like "normal" PyTorch training code as possible. The current code, for a skorch novice, would look quite esoteric: what is notify? what is TeeGenerator??

[githubnemo]

I suspect that changing the signature of callbacks would break's users code the most.

Agreed. Not every user might follow the changelog closely, we could wrap the notify call with an exception handler and in case of on_batch_begin we could raise the original exception in addition to a note informing the user about the change in signature and how to fix it. This could stay there for one release.

[BenjaminBossan]

@ottonemo I'd like to refine #707 but I think there might be some conflicts with this PR, so we should probably decide on whether to go forward with this one or not. I think the biggest hurdle was the question whether we can provide a useful exception when a user needs to change the signature. Did you see my comment on this?

[BenjaminBossan]

@ottonemo I implemented your suggestion to check for user-defined callbacks and issue a warning containing the required fix and a note on how to turn off the warning. On top of your suggestion, I also check if any of the user-defined callbacks even overrides on_batch_{begin,end} and don't issue the warning if they don't.