swav-improvements

[Atharva-Phatak]

What does this PR do?

Quality checks for implementations of SWAV #839

Couple of points I would like to highlight.

• Right now bolts is focused on standard datasets like CIFAR10, etc but it will be very amazing if we can adapt SSL module for custom tasks.
• The SSL fine tuner is just a normal lightingModule we can remove it and let the user create one according to his/her needs. We will just provide the basic models and functionalities required for implementing SSL tasks.
• These changes will require a substantial rewrite and we can discuss this in further issues.

Fixes part of #839

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests? [not needed for typos/docs]
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

• Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[Atharva-Phatak]

@krshrimali Can you trigger other runs ?

[krshrimali]

Done, @Atharva-Phatak! 🚀

[Atharva-Phatak]

@krshrimali Another run please.

[krshrimali]

@krshrimali Another run please.

Done! Sorry for the delay. :(

[Atharva-Phatak]

@krshrimali Another run please.

Done! Sorry for the delay. :(

Hey, may I know the reason why I have to ask you or maintainers to trigger the run ? Is there anyway around this. I feel bad pinging you everytime.

[krshrimali]

@krshrimali Another run please.

Done! Sorry for the delay. :(

Hey, may I know the reason why I have to ask you or maintainers to trigger the run ? Is there anyway around this. I feel bad pinging you everytime.

Hi Atharva! As far as I remember, until your very first PR is merged, it's expected that a maintainer will have to approve the tests to run. I remember I had to do it myself as well, pinging my mentor all the time :D I think, the reason it makes sense is, is to prevent any one to spam our CI. A lot of resources go to each CI in the Lightning Ecosystem, hence this defense mechanism 😀

But don't feel bad about it, if you are on PL slack, just message me there if that helps, my username is: krshrimali (I hope xD else just search with Kushashwa). Whenever you want to trigger the run, ping me there and I will do it.

Once your first PR is merged, you will not need me :))

Also, is there a way around it? Uhmmm, I am not very sure if there is. I will let @otaj answer this one. 🤎

[Atharva-Phatak]

@krshrimali Another run please.

Done! Sorry for the delay. :(

Hey, may I know the reason why I have to ask you or maintainers to trigger the run ? Is there anyway around this. I feel bad pinging you everytime.

Hi Atharva! As far as I remember, until your very first PR is merged, it's expected that a maintainer will have to approve the tests to run. I remember I had to do it myself as well, pinging my mentor all the time :D I think, the reason it makes sense is, is to prevent any one to spam our CI. A lot of resources go to each CI in the Lightning Ecosystem, hence this defense mechanism grinning

But don't feel bad about it, if you are on PL slack, just message me there if that helps, my username is: krshrimali (I hope xD else just search with Kushashwa). Whenever you want to trigger the run, ping me there and I will do it.

Once your first PR is merged, you will not need me :))

Also, is there a way around it? Uhmmm, I am not very sure if there is. I will let @otaj answer this one. brown_heart

@krshrimali I am going to ping you a lot haha :). I really want to join the lightning team.
I hope I will get the opportunity.

[Atharva-Phatak]

@otaj @krshrimali All tests are passing, finally. Hopefully this will be merged :)

[otaj]

Hi, @Atharva-Phatak, thank you very much! First of all, I'm very sorry for me being so delayed, thank you @krshrimali for stepping in!

Regarding the code, overall it looks good, however, I'm don't see much of a difference between swaw_resnet.py module and other resnet module we already have in the code, such as models.self_supervised.resnets, do you think you can clarify it? Ideally, we don't want every sing model implement resnets (or anything else, for that matter) from scratch.

[otaj]

Regarding if there's a way around maintainers needing to trigger CI checks, I'm afraid, there isn't. This is a protection against things like spamming, crypto mining in the CI and all of the ugly things regarding spammers having access to "free" compute. I believe the first merged commit is a reasonable filter.

[Atharva-Phatak]

@otaj Regarding resnets, The implementation of main class ResNet in SWAV is little different as it has multiple heads called as prototype heads. Hence I have not changed it. I agree that every single module should not implement resents, but considering this is an initial iteration I recommend we keep it that way. Once this major revision is complete.
I can take over the whole of SSL module as its not very customizable (in terms of models and datasets).
The reason why I am recommending this is many people are working on various modules, transforms, etc and we can discuss a major revision of SSL in bolts as it very much required because I think SSL is very important. Let me know what are you thoughts.

[Atharva-Phatak]

@otaj Any updates ? Can this be merged ?