[Question] Why resnets in bolts ? What is the difference with torchvision models ?

[YannDubs]

❓ Questions and Help

Hi it seems to me that all the resnets are the same as in torchvision, I tried to look at why have essentially duplicate code and which ones I should be using but I couldn't find any discussion about that. So my questions are :

1. Are the models EXCATLY the same ?
   1.1. If not what are differences and which should I be using ?
   1.2. If yes, why the code duplication ?

thanks :)

[github-actions[bot]]

Hi! thanks for your contribution!, great first issue!

[akihironitta]

@YannDubs Basically, models like RetinaNet (being added in #529 by @oke-aditya) are the same as torchvision's ones as you might have noticed. Wrapping torchvision models with lightning allows us to utilise lightning features, e.g. trainer, callback, logger, ...

[oke-aditya]

I think @YannDubs is talking about resnets file.

For self supervised learning I thnk we have duplicated the code for ResNets.

Edit. Just noticed that we have copy pasted major ResNet code in autoencoders components too

This code duplication was done to avoid dependency on torchvision.

While in detection we did exact opposite, I used torchvision.models to create ResNet FPN Backbones,.

This is still a prevailing question. Our dependency on PyTorch and torchvision.

I think we can discuss more. I would love to hear your thoughts ! @YannDubs @akihironitta

[YannDubs]

Thanks for the clarification! I was indeed talking about the resnets file.

I agree that bolts shouldn't require on torchvision (for example if It is used for NLP), but I think that the parts that are related to computer vision should use as much as possible torchvision (and raise some error if torchvision is missing). I.e. I would have done it exactly as was done for detection.

In any case, most of the other components of the self-supervised code (e.g. image normalization) requires torchvision, so I don't see the point of having so much code duplication.

[oke-aditya]

Hi @YannDubs I do agree on the points.

I agree that parts related to computer vision might need torchvision.
Torchvision is very stable and a lot of projects do depend on it.
E.g. Classyvision, Detectron2
Torchvision takes a lot of care while upgrading and usually avoids BC Breaking changes.
Also, torchvision is pinned to the latest PyTorch version. E.g. Pytorch 1.7 is pinned to torchvision 0.8 and
PyTorch 1.8 is pinned to torchvision 0.7

Here are a few solutions I propose.

Solution 1

Pin Versions of PyTorch, Torchvision, PyTorch Lightning for PL bolts together.

This is what PyTorch does for all libraries, torchvision, torchtext and torchaudio. Also is frequently followed practice.

E.g. We could possibly make a version and keep it compatible with particular PyTorch and torchvision

Bolts Version	PyTorch Version	PyTorchLightning Version	Torchvision version
0.3	1.6	1.1	0.7
0.4	1.7	1.2	0.8
0.4.1	1.7.1	1.2.1	0.8.1

Note that Torchvsiion is not mandatory for bolts installation but if people need torchvision they need to follow the above compatibility matrix.

Solution 2 Lazy load torchvision

We would need to inform users that particular functionality is not available In the user installed torchvision version.

An example of this can be RetinaNet #529 #391 . User is free to install any torchvision with bolts, but if he tries to import/use RetinaNet then we would raise an error that his version of torchvision does not include this feature.

This is possible but, this can lead to compatibility issues and a lot of maintenance! E.g Bolts 0.3 would have to support multiple torchvision versions as well as handle such Frequent issues. Torchvision does not do a lot of BC Breaking changes so I'm not very sure if this solution can be good. This is fine when project is small but moving forward it will be lot of error checking and testing, it is really overhead to maintain specific components to specific torchvision versions.

Solution 3 Make bolts Independent from Torchvision.

Well, this is re-inventing the wheel.
There are few caveats in torchvision, but really rewriting most code such as ResNets, operations, utilities would take a lot of time.
Though not impossible, but this is major copy paste and maybe we will face issues in licenses etc. So really not wise.

I would love to hear thoughts from all ! cc @akihironitta @Borda @ananyahjha93 @YannDubs !

[YannDubs]

I think that's a very good summary of possibilities. I don't know bolts as much as you all do, but it definitely seems that solution 1 is preferable. Are there any disadvantages of doing that ?

[akihironitta]

@YannDubs @oke-aditya Sorry for the delay, and thank you both for sharing your thoughts!

So, as far as I understand, there are two issues here:

1. whether we use torchvision's implementation or have our own implementation. (e.g. utilities, models, components...)
   In my opinion, we should depend on torchvision because we can reduce the cost of maintenance by using such well-maintained libraries.

2. how we handle torch and torchvision versions.
   If I understand it correctly, some of the features like Add RetinaNet Object detection with Backbones #529 in Bolts needs torchvision>=0.8 which needs torch>=1.7, right? I still don't see why we should raise the min versions only for a few Bolts' features which don't affect many users. We can just throw an error with a message like "To use this feature, upgrade torchvision>=0.8", can't we? So, +1 for "Solution 2 Lazy load torchvision".
   This is also what @Borda mentioned in Add RetinaNet Object detection with Backbones #529 (comment) in

   we do not force users to upgrade their env unless it is critical for all package

@oke-aditya I might be missing something, but could you elaborate more on why there will be compatibility issues and increase the cost of maintenance? because it seems to me that users can use the new features by upgrading torchvision>=0.7 to torchvision>=0.8 (and thus to torch>=1.7) without any problem.

this can lead to compatibility issues and a lot of maintenance!

[oke-aditya]

Hey @akihironitta thanks for having a look. Here are my thoughts on why lazy loading would cause pains.

1. Torchvision internal critical bug fixes.

To use these features, we would have to use torchvision. As we want to avoid the cost of maintenance from our side.
Now since these change internally in torchvision, we don't have much control. So we would end up having users not have consistency.

2. Upgrading is easy for users!

If we follow the compatibility matrix, we can upgrade easily and still stay very stable. To use features users will have to just use a previous release. They don't have a confusion either and solves later issues. Assume a user thinks he can use RetinaNet out of box, later to release he needs to bump his versions. Well, this is a pain for some users and really we could be clear at first palce.

We aren't suddenly upgrading. We are continuously providing a release for every PyTorch and torchvision as well as Lightning. Hence a relief for both parties.

3. Torchvision is backward compatible

It would naturally warn users before deprecation and upgrading is never a big issue.

4. Complicates the codebase.

Currently, we faced the issue from 0.7 -> 0.8. This will keep growing, some features would be available from 0.9 and hence we would have to warn again at places for these. And finally, when we upgrade the library, we would have to remove these errors.
This is double work which can easily be avoided.
At first it looks small but as the codebase grows, this solution will dig more problems.

5. Duplication of code.

What if there is a small feature in new torchvision release (like ops) and we want to stay compatibly for old users. We can't upgrade but again copy-paste code which is not nice.

A proper release cycle for lightning compatibitliy and torchvision is really easier to maintain and makes even coding easier.

The first solution is pretty neat and in my opinion is easier to implement.
We are not forcing users to upgrade ! They just need to follow compatibility matrix. If they are fine with torchvision they have, we have a corresponding bolts for it!

[akihironitta]

@PyTorchLightning/core-bolts Any thoughts?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.