Callbacks don't work when passed unitialized

[ottonemo]

For example:

class Foo(inferno.callbacks.Callback):
    def on_epoch_end(self, net, **kwargs):
        pass

net = NeuralNet(..., callbacks=[Foo])

Error:

Traceback (most recent call last):
  File "train.py", line 189, in <module>
    pl.fit(corpus.train[:1000], corpus.train[:1000])
  File "/home/ottonemo/anaconda3/lib/python3.6/site-packages/sklearn/model_selection/_search.py", line 945, in fit
    return self._fit(X, y, groups, ParameterGrid(self.param_grid))
  File "/home/ottonemo/anaconda3/lib/python3.6/site-packages/sklearn/model_selection/_search.py", line 550, in _fit
    base_estimator = clone(self.estimator)
  File "/home/ottonemo/anaconda3/lib/python3.6/site-packages/sklearn/base.py", line 69, in clone
    new_object_params[name] = clone(param, safe=False)
  File "/home/ottonemo/anaconda3/lib/python3.6/site-packages/sklearn/base.py", line 57, in clone
    return estimator_type([clone(e, safe=safe) for e in estimator])
  File "/home/ottonemo/anaconda3/lib/python3.6/site-packages/sklearn/base.py", line 57, in <listcomp>
    return estimator_type([clone(e, safe=safe) for e in estimator])
  File "/home/ottonemo/anaconda3/lib/python3.6/site-packages/sklearn/base.py", line 67, in clone
    new_object_params = estimator.get_params(deep=False)
TypeError: get_params() missing 1 required positional argument: 'self'

Probable cause: get_params recursively inspects all attributes of the wrapper instance including self.callbacks which still contains the uninitialized callbacks. It then calls get_params which does not work as it is not a static method.

[benjamin-work]

I couldn't reproduce it:

def test_get_params_with_uninit_callbacks(self, net_cls, module_cls):
    from inferno.callbacks import EpochTimer

    net = net_cls(
        module_cls,
        callbacks=[EpochTimer, ('other_timer', EpochTimer)],
    )
    # does not raise
    net.get_params()
    net.initialize()
    net.get_params()

Was this accidentally fixed?

[benjamin-work]

Okay, this is how to reproduce it:

        net = net_cls(
            module_cls,
            callbacks=[EpochTimer, ('other_timer', EpochTimer)],
        )
        net = clone(net)

[benjamin-work]

That problem cannot be easiliy fixed it seems. We could not return callbacks when get_params is called on NeuralNet, thus avoiding the recursive clone call on the un-initialized callbacks. However, that would mean that the cloned object does not have the callbacks anymore!

Specifically, the problem occurs on all attributes that are un-initialized and have the get_params method (which sklearn looks for). At the moment, this only applies to the callbacks.

I believe this is one further argument in favor of requiring everything to be initialized (see #13 )

[ottonemo]

I believe this is one further argument in favor of requiring everything to be initialized (see #13 )

Yeah. I think this is a WONTFIX in favor of #13.

[benjamin-work]

Agree

[fcharras]

Came here to report the same bug.

Doesn't passing callbacks initialized break sklearn compatibility ? the inner state of the callbacks will change during training, which contradicts: These initial arguments (or parameters) are always remembered by the estimator.

A workaround to this could be to store the uninstancied callbacks in a special container that inherits from lists ?

class CallbackContainer(list):
pass

(I quickly checked that this does preserve clone and initialize_callbacks methods)

This is still a slight modification of the input argument but it seems acceptable.

Indeed the type checking in clone is strict

[githubnemo]

Thanks for chiming in.

Doesn't passing callbacks initialized break sklearn compatibility ? the inner state of the callbacks will change during training, which contradicts: These initial arguments (or parameters) are always remembered by the estimator.

I don't think that skorch callbacks violate this the way they are implemented. It is true that they change their state and it is also true that this mutates the initial arguments. However, once training starts again or the network is initialized the callbacks are re-set to their initial state exactly the way they were initalized (via the .initialize() method of the callback). If a callback has internal state it is implemented the sklearn way (self.foo_ for the changing parameter, self.foo for the initial parameter) to avoid this problem.

In my problem there is no issue but maybe I'm overlooking something. Is there a particular reason that you are reporting this right now?

[BenjaminBossan]

Just to add to @githubnemo's comment, think of callbacks as an sklearn pipeline. When you fit the pipeline, the estimators are mutated. However, their __init__ arguments remain untouched. And if you fit a second time, you should get the same result. This behaviour is the same behaviour as callbacks have. And analogously, pipelines don't allow to pass uninitialized estimators.

For that reason, I would leave the restriction except if you can show us a situation where this is undesired.

[fcharras]

Thank you for the clarification, I understand your points. I think it is debatable whether this is compatible with the sklearn coding guidelines or not. I'd like to add a few more remarks including my usecases.

Before that, the issue for me is not strictly that the current NeuralNet does not support uninstanciated callbacks, it is that objects passed as input are mutated. If it were possible to clone the callbacks in initialize_callbacks before use it would also solve the issues.

think of callbacks as an sklearn pipeline

except sklearn.pipeline.Pipeline (and FeatureUnion) do not inherit from BaseEstimator, it's a different beast (a _BaseComposition). I'd imagine that a NeuralNet is not a composition, but an estimator (and a meta estimator).

I see two practical issues with instanciated callbacks passed as arguments:

• in some cases, sklearn relies in hashing the input arguments stored as attributes to get a unique index of the estimator, for caching purpose. While I think that the caching utility in sklearn is really rough, current NeuralNet is not compatible with it, because the hash will differs before and after training. It's not a problem with sklearn' metaestimators because instanciated estimators are hashable and always cloned before use.
• it's harder to keep track of objects attached to callbacks when trying to clean things. I've had usecases where the references in callbacks and the references in callbacks_ diverged (because I saved and loaded callbacks) and it took me some time to understand why the RAM wouldn't empty (because of references attached in callbacks).

(On a side not, speaking of references to old objects that should be flushed, it is not good to store a reference to the optimizer in LR schedulers like ReduceLROnPlateau, because if the optimizer is reinitialized during training, the reference in the scheduler is not updated. For my use I've rewritten those callbacks to avoid such an issue, in general I think that, for all objects that should remain synchronized or that might weight in RAM, it should be carefully ensured that only one reference exist at all time, and this reference is attached to an attribute of the current NeuralNet instance, and any interaction with those objects are done with this unique reference)

[ottonemo]

As you already said, this is not about callbacks being uninitialized but rather about potential side-effects.
Let's discuss this in a separate issue #434.

it is not good to store a reference to the optimizer in LR schedulers like ReduceLROnPlateau

I agree with you that storing references is problematic and should be avoided when possible. In this case,
there is not much we can do about it, though, since it is the PyTorch API we are dealing with, not skorch's. We also take care to make sure that only the latest reference is used when creating the scheduler before training to avoid such issues. If there is an issue that we have overlooked, please report it so we can work on a solution.