Require uninitialized optimizers for our learners

[timokau]

Description

An initialized optimizer is a tensorflow object, which (at least in
graph mode in tf1) is not deepcopy-able. Even if we were able to
deeocopy it, we probably wouldn't want to since it contains state.
Scikit-learn needs to be able to deepcopy an estimators arguments so
that it can create copies and derivatives of it.

Instead we require the uninitialized optimizer and its parameters to be
passed to our learners separately. The learner can then initialize the
optimizer as needed.

I have only done this for CmpNet so far. We'd have to do it for all estimators that take an optimizer (i.e. pretty much all of them). Before I do that, I would like some feedback on the general approach. I went with a function with out arguments since that retains the most flexibility. It may be a bit weird to users that want to override the optimizer though. An alternative would be to pass the name of a keras optimizer and a configuration dict. That would be less "weird", but also less flexible since users could no longer write their own custom estimators. @kiudee @prithagupta any thoughts?

Motivation and Context

Continuation of #116.

How Has This Been Tested?

Ran the test suite and the pre-commit hooks.

Does this close/impact existing issues?

#94, #116

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.

[timokau]

Rebased.

[codecov]

Codecov Report

Merging #119 into master will increase coverage by 0.12%.
The diff coverage is 84.44%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   59.96%   60.08%   +0.12%     
==========================================
  Files         116      116              
  Lines        7658     7662       +4     
==========================================
+ Hits         4592     4604      +12     
+ Misses       3066     3058       -8     

Impacted Files	Coverage Δ
csrank/choicefunction/cmpnet_choice.py	84.78% <ø> (ø)
csrank/choicefunction/fate_choice.py	91.30% <ø> (ø)
csrank/choicefunction/feta_choice.py	66.00% <ø> (ø)
csrank/choicefunction/ranknet_choice.py	81.63% <ø> (ø)
csrank/discretechoice/cmpnet_discrete_choice.py	86.84% <ø> (ø)
csrank/discretechoice/fate_discrete_choice.py	94.44% <ø> (ø)
csrank/discretechoice/feta_discrete_choice.py	55.00% <ø> (ø)
csrank/discretechoice/ranknet_discrete_choice.py	84.61% <ø> (ø)
csrank/objectranking/cmp_net.py	87.17% <ø> (ø)
csrank/objectranking/fate_object_ranker.py	93.33% <ø> (ø)
... and 16 more

[kiudee]

I took a look at what skorch (a scikit-learn wrapper for PyTorch) is doing. There solution can be found here:
https://github.com/skorch-dev/skorch/blob/a621d0fa31c87adbdbb3b297328caefb09d42b8c/skorch/net.py#L504-L535
Basically, they allow users to pass in an uninitialized class, which will be initialized on fit. The user can specify parameters using the nested __ notation (which turns out to be the official way they do it in scikit-learn).
An alternative would be as you say to pass in a configuration dictionary.

[kiudee]

One big consideration here is hyperparameter optimization. The parameters of an optimizer (e.g. learning rate) are often optimized jointly with other parameters of the learner.

So an important use-case is for instance that RandomSearchCV is run on the model.
The way this is organized in skorch and scikit-learn, a user would be able to pass something like:

{
    'module__n_hidden_layers': ...,
    'module__n_hidden_units': ...,
    'criterion__param1': ...,
    'optimizer__learning_rate': ...,
}

And the hierarchy there can be nested.

[timokau]

I took a look at what skorch (a scikit-learn wrapper for PyTorch) is doing. There solution can be found here:
https://github.com/skorch-dev/skorch/blob/a621d0fa31c87adbdbb3b297328caefb09d42b8c/skorch/net.py#L504-L535
Basically, they allow users to pass in an uninitialized class, which will be initialized on fit. The user can specify parameters using the nested __ notation (which turns out to be the official way they do it in scikit-learn).
An alternative would be as you say to pass in a configuration dictionary.

The __ approach seems hacky to me. There is a natural way to pass nested configuration in python: dicts.

One big consideration here is hyperparameter optimization. The parameters of an optimizer (e.g. learning rate) are often optimized jointly with other parameters of the learner.

Of course, I don't know why I didn't think of that. That's a very good reason not to use the initializer-function approach I demonstrated in this PR. Good thing I waited for feedback first.

So an important use-case is for instance that RandomSearchCV is run on the model.
The way this is organized in skorch and scikit-learn, a user would be able to pass something like:

{
    'module__n_hidden_layers': ...,
    'module__n_hidden_units': ...,
    'criterion__param1': ...,
    'optimizer__learning_rate': ...,
}

And the hierarchy there can be nested.

It looks to me like the __ thing is a hack around the limitations of scikit-learn's hyperparameter optimization. From a brief skim over their documentation, it seems like there is no easy way to define distributions over dictionary-nested parameters. You would have to define a custom distribution function.

If that was more easily possible, you could also define different sets of hyperparameters for different optimizers and then hierarchically pick an optimizer first, then pick the hyperparameters for the optimizer.

Still, the design of scikit-learn combined with the fact that it seems to be the standard seems like a strong argument in favor of __. So even though I think dict-based configuration would be more elegant, we probably should go ahead with the sktorch approach. Do you agree?

[kiudee]

Indeed, that also felt very hacky to me, too.
But I also agree that it is something people are used to and which is not that difficult to adapt to as a user. So I think we should follow that.

[timokau]

I have pushed a proof of concept. It doesn't actually quite pass the tests yet, but it shows two issues with this approach when specifying default optimizer arguments:

• adds a lot of boilerplate for passing these around
• how should we handle the case when the user picks a different optimizer that doesn't take the nesterov argument?

The first one could probably be worked around by centralizing our defaults (which we should do anyways I think), but the second one is a bit ugly.

[timokau]

This is what I am talking about.

[timokau]

I see some possible solutions:

1. Simply drop the nesterov and momentum defaults. That is what skorch does: They only provide a default for lr and rely on the optimizer's defaults for other parameters.
2. Provide a parallel mechanism of passing optimizer parameters, e.g. an optimizer_config: dict attribute. We could supply a default for that which is then overridden by any manual changes. It would likely be confusing to merge & use the two configuration mechanisms though.
3. Set the default optimizer to None and specify in the docs that None refers to SGD with our default options.
4. Try to figure out which parameters apply to which optimizer, using introspection. This is hard to get right and brittle.

I think all in all, the most elegant approach would be to fall back to either the lambda-parameter or a tuple (optimizer, param_dict). Given that we want to use the __ approach, solution (1) seems best to me. Is there any particular reason that we're overriding the optimizers defaults here?

Are there other options I'm not seeing?

[timokau]

If we want to keep the parameters, (3) is probably best (we could also chose a string instead of None to represent that default). Or we could define our own optimizer, which is just SGD with those changed defaults.

[kiudee]

I agree that the cleanest solution would be (1) here. The only reason I see why we could provide different defaults is that we think that these work better out of the box.
But that is hard to guarantee in general.

[timokau]

So should I go ahead then and remove the nasterov and momentum defaults? What about the learning rate? Its always set to 1e-4. The default is 1e-2, so maybe that is too high for our purposes.

[timokau]

If we want to keep lr that is probably okay, since I expect all optimizers will accept a lr argument. Our default may not make sense for any optimizer though, and it may be somewhat cleaner to be "consistent" in not overriding the optimizers defaults.

[kiudee]

I would say we can leave all of those defaults to the corresponding optimizers. Usually, you need to adapt the parameters to your specific problem anyways.

[timokau]

I moved that step to #142.

[timokau]

I started with the transition, but there is still quite a bit of work to be done. Still WIP.

[timokau]

The testsuite passes now. I had to disable one test though where the reason for failure was not obvious (FATELINEAR_DC). I'll have to look into that in more detail. Some cleanup is needed as well.

[timokau]

The FATELINEAR_DC issue was straightforward after all. Only documentation & a self-review remaining.

[timokau]

Ready for review. This is a breaking change.

[timokau]

Switched the milestone following semver.

[kiudee]

This looks good to me.

Since we now break backward compatibility, but still might want to backport patches for 1.x.y, we have to think about establishing legacy branches, which travis-ci also deploys from.

[timokau]

Thanks for the review!

Since we now break backward compatibility, but still might want to backport patches for 1.x.y, we have to think about establishing legacy branches, which travis-ci also deploys from.

Yes, that'd be something to explore. Branches could always be created retroactively though once we actually have to ship some fix.

[timokau]

Speaking of travis, there seems to be something wrong with the github integration. The check in github is still marked as in progress, but when clicking on "Details" travis tells you that its long finished. Is it possible to restart the job?

[kiudee]

After restarting the job, it fails now with:

'Keras requires TensorFlow 2.2 or higher. '

[timokau]

I have pinned keras and travis is happy. Please have another look.

[timokau]

It turns out that the optimizer=uninitialized_optimizer pattern still doesn't pass scikit-learn's estimator check. There seems to be no good reason for this though, I did some digging and opened an upstream issue: scikit-learn/scikit-learn#17756

I think this is still good to merge. The test is likely just overly restrictive.