Base class for models

[kazhang]

🚀 The feature

Introduce a base class for all TorchVision models. The base class defines common interfaces and attributes.

Motivation, pitch

We can track the model instantiation similar to dataset instantiation.
We can also define a reset_parameters interface to address #3410.

Alternatives

Adding logging and reset_parameters to each of the models manually. The approach is not future proof, we might forget to do that for new models.

Additional context

No response

cc @datumbox

[datumbox]

I think it's a reasonable idea that will provide structure on the API of models. I assume only the main classes of the models will implement this API, not submodules like Bottleneck etc, right? Another thing that might worth clarifying in this RFC is how reset_parameters will work for models that contain backbones (such as detection and segmentation).

[vadimkantorov]

In my opinion, if there's not much functionality that's best inherited (e.g. reset_parameters could even be introduced as an empty method on Modules since it's so core and frequent or just be used with duck typing), it's better to keep the inheritance as simple as possible since users are reading this code as example for their own models, so it's best to avoid making it more complex unless absolutely necessary/helpful.

[datumbox]

reset_parameters could even be introduced as an empty method on Modules since it's so core and frequent

That would be nice but it might be hard to sell and take a very long time to coordinate.

there's not much functionality that's best inherited

I agree with this. The only reason to introduce such an object would be to throw an NotImplementedError error and try to force implementations to always put the module initialization in the right place. There are definitely pros/cons here and the extra complexity is definitely a good point to consider.

An additional issue that ties with both your points and my earlier comment is, what happens with submodules like Bottleneck that also initialize weights? Shouldn't they also have reset_parameters methods? Also how about nested models like backbones? I think it's worth clarifying these points before we decide one way or the other.

[vadimkantorov]

About submodules and recursiveness, there are similar discussions going on in the main repo: pytorch/pytorch#66317 / pytorch/pytorch#65153

With respect to recursiveness, IMO at least there should be a method at the highest level (i.e. the model), and then if makes sense it could move init code to submodules. This already should allow starting unification of init code under a single name reset_parameters and it would already become useful, and then if there is a decision, it could be renamed or default recursiveness could be added later

[kazhang]

I agree that we should avoid unnecessary inheritance so that the implementation is more readable. I open this issue to gather generic functionalities that should be applied to all models.

should submodules implement the reset_parameters method?
I hope not, the submodules are usually constructed before the main class, even if they implement the reset_parameters, their initialization will be reset by the reset_parameters of main class, unless we introduce extra complexity to either

• check if a submodule has been initialized and filter if so.
• Alternatively, iterate through submodules and check if it has reset_parameters, if not, use the default initialization.

Is there a need to initialize the submodule differently in real world? otherwise, I would recommend only the main class should implement the reset_parameters method.
models with backbones
I think backbone is equivalent to submodule.

[fmassa]

I think I would maybe do a step back and consider what functionality we would want to have in a base VisionModule class.

I agree with @vadimkantorov that it might be best to keep things as simple as possible.

An example of base inheritance making things harder for the user is the VisionDataset, which does some magic internally to keep BC and handle a few other use-cases.

Without some strong need for a base class, I would lean towards keeping everything basic nn.Module.

[datumbox]

@kazhang Thanks for the clarifications. Agreed.

I open this issue to gather generic functionalities that should be applied to all models.

Good idea, worth discussing.

Since there seems that there is not enough generic functionalities to be shared across models and there is agreement to avoid introducing unnecessary complexity, does it mean we can close the issue?

[kazhang]

Let's close this issue for now until we find strong enough feature set to be added to a generic top-level class.