Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Major revision of Bolts #839

Open
otaj opened this issue Jul 21, 2022 · 59 comments
Open

Major revision of Bolts #839

otaj opened this issue Jul 21, 2022 · 59 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed let's do it! Looking forward to have it implemented

Comments

@otaj
Copy link
Contributor

otaj commented Jul 21, 2022 •
edited by Atharva-Phatak

Let's revision Bolts and breathe some fresh air into them! As outlined in #819 and on a Slack channel, we will revisit every single feature within Bolts.

Please sign up for a feature which you'd like to tackle. Once you do so, I will attach your name in the list and you will be expected to open a PR within two weeks. It might be useful to tackle multiple things at once as the feature list is everything top-level from the repository.

Criteria

The criteria for acceptance are simple:

  1. Feature has to be properly tested. Unit tests are essential, integration tests are also good, but we understand there might be situations when they are impossible.
  2. Feature has to be compatible with the latest set of Lightning libraries (lightning, flash, torchmetrics). This fact is also tested. This also means outputting things in formats consumable by other projects, namely Flash.
  3. Feature cannot be duplicated across our codebase, codebase of other Lightning projects and other more mature OSS projects. For example, there are currently five functions/classes that implement conv3x3 in this repository.
  4. Feature does not raise any warnings in tests. For this, there is a pytest fixture in Introduce catch_warnings fixture #844 (please, read Major revision of Bolts #839 (comment) for the better understanding)
  5. And of course, once you're done, remove the decorator @under_review from the selected feature 🎉

List of features to be reviewed

Features

Thank you for your contributions! 💪 🚀 🎉
@otaj otaj added fix fixing issues... help wanted Extra attention is needed good first issue Good for newcomers let's do it! Looking forward to have it implemented and removed fix fixing issues... labels Jul 21, 2022
@otaj otaj pinned this issue Jul 21, 2022
@oke-aditya
Copy link
Contributor

oke-aditya commented Jul 22, 2022 •
edited

While adding new features is good. But what do you think of re using them from torchvision?

E.g for g / d / c IoU loss are already in torchvision

@otaj
Copy link
Contributor Author

otaj commented Jul 22, 2022

If an appropriate feature is available in other projects and is in a stable state (i.e. the package isn't in beta or the feature itself is not beta/unstable/experimental), I'd go for using such feature from the other projects.

This might get tricky with new packages as we do not want to have an endless list of dependencies. But, in case of torchvision, we already require it for models, so that is absolutely ok.

@oke-aditya
Copy link
Contributor

Then in torchvision.ops we already have all 3. Operations and losses.

This is from v0.13 latest one.

@Ce11an
Copy link
Contributor

Ce11an commented Jul 25, 2022

Hey @otaj,

I am happy to work on the following to start as they are connected:

  • pl_bolts.datamodules.sklearn_datamodule.SklearnDataModule
  • pl_bolts.datamodules.sklearn_datamodule.SklearnDataset
  • pl_bolts.datamodules.sklearn_datamodule.TensorDataset
  • pl_bolts.models.regression.linear_regression.cli_main
  • pl_bolts.models.regression.linear_regression.LinearRegression
  • pl_bolts.models.regression.logistic_regression.cli_main
  • pl_bolts.models.regression.logistic_regression.LogisticRegression

If that is all good with you? Thanks! 😆

@BaruchG
Copy link
Contributor

BaruchG commented Jul 25, 2022

Hey @otaj,
I'm going to start off with the hopefully simple pl_bolts.datasets.cifar10_dataset.CIFAR10 and once I get the hang of the process I'll start doing it in batches.

@krishnakalyan3
Copy link

krishnakalyan3 commented Jul 25, 2022 •
edited

I would like to work on

pl_bolts.models.autoencoders

Follow-up questions on removing optimization policies.

@luca-medeiros
Copy link
Contributor

Cool stuff!
I would like to work on pl_bolts.callbacks.data_monitor.

@ArnolFokam
Copy link
Contributor

ArnolFokam commented Jul 26, 2022 •
edited

Hi @otaj,

Would like to work on features around SimCLR

  • pl_bolts.models.self_supervised.simclr.simclr_finetuner.cli_main
  • pl_bolts.models.self_supervised.simclr.simclr_module.cli_main
  • pl_bolts.models.self_supervised.simclr.simclr_module.Projection
  • pl_bolts.models.self_supervised.simclr.simclr_module.SimCLR
  • pl_bolts.models.self_supervised.simclr.simclr_module.SyncFunction
  • pl_bolts.models.self_supervised.simclr.transforms.GaussianBlur
  • pl_bolts.models.self_supervised.simclr.transforms.SimCLREvalDataTransform
  • pl_bolts.models.self_supervised.simclr.transforms.SimCLRFinetuneTransform
  • pl_bolts.models.self_supervised.simclr.transforms.SimCLRTrainDataTransform

@otaj
Copy link
Contributor Author

otaj commented Jul 26, 2022

Oh my! I go to sleep, and suddenly, it blows up! Thank you, @Ce11an, @BaruchG, @krishnakalyan3, @luca-medeiros, @ArnolFokam! I added all of you to the list ⚡ 🔩

As to whether we should remove LARS and LinearWarmupCosineAnnealingLR - yes, but with a deprecation warning. I will take them on myself.

@redleaf-kim
Copy link
Contributor

I'd love to contribute this awesome project.
I would like to work on pl_bolts.models.detection.yolo

Cheers

@otaj
Copy link
Contributor Author

otaj commented Jul 26, 2022

Hi, @heimish-kyma, awesome! ⚡ I've added you to the list 💪

@shivammehta25
Copy link
Contributor

shivammehta25 commented Jul 26, 2022 •
edited

Hello! Can I try helping with pl_bolts.models.gans.basic.* ?

@otaj
Copy link
Contributor Author

otaj commented Jul 26, 2022

Awesome, @shivammehta007, thank you! I added you to the list ⚡

@shivammehta25 shivammehta25 mentioned this issue Jul 26, 2022
Merged
8 tasks
@otaj
Copy link
Contributor Author

otaj commented Jul 27, 2022

Hi everyone, but especially those who already signed up for work (@shivammehta007, @heimish-kyma, @ArnolFokam, @luca-medeiros, @krishnakalyan3, @BaruchG, @Ce11an). Please excuse me for "changing the rules" while you're already signed up for work, but this is very much a learning experience for me as well 🧑‍🏫 Let's all consider #843 a "testing PR" where we can iterate on the process of how will it look like in the end. 🔩

In order to ensure stability and compatibility, we'd like to not raise any warnings in the tests (other than UnderReviewWarning, and ideally not even that one). For this reason, I just opened #844, which is a simple fixture raising errors on warnings.

Please note, that it can happen, that these warnings are raised in other features which you haven't signed up for and there are potentially numerous solutions to that:

  1. If that feature is not taken up by anyone and you're up for it, take it up! 💪
  2. If that feature is not taken up by anyone, but you don't want to take anymore features, ping me either in some (appropriate) issue on GH or on Slack (slack username on PL is @Ota) and we'll figure something out ⚡
  3. If that feature is taken up by someone already, ping them about it (and maybe include me in the loop as well). 🚀

Thank you! 🔩 ⚡ 💪

@shivammehta25
Copy link
Contributor

Can I also take pl_bolts.datamodules.mnist_datamodule. MNISTDataModule , pl_bolts.datamodules.vision_datamodule.VisionDataModule and pl_bolts.datamodules.cifar10_datamodule.*, ?

@otaj
Copy link
Contributor Author

otaj commented Jul 27, 2022

@shivammehta007, absolutely! Thank you, you were added to the list!

@wonbbnote
Copy link

Hi, I would like to work on pl_bolts.datamodules.kitti_datamodule.KittiDataModule!!

@Ce11an Ce11an mentioned this issue Jul 31, 2022
Closed
8 tasks
@otaj
Copy link
Contributor Author

otaj commented Aug 2, 2022

Hi @wonbbnote, thank you very much, you were also added to the list!

@luca-medeiros luca-medeiros mentioned this issue Aug 2, 2022
Open
12 tasks
@nishantb06
Copy link
Contributor

Hi @otaj ,
Would like to work on features related to dummy datasets -

  • pl_bolts.datasets.dummy_dataset.DummyDataset
  • pl_bolts.datasets.dummy_dataset.DummyDetectionDataset
  • pl_bolts.datasets.dummy_dataset.RandomDataset
  • pl_bolts.datasets.dummy_dataset.RandomDictDataset
  • pl_bolts.datasets.dummy_dataset.RandomDictStringDataset

I Plan to improve tests and documentations for these datasets.

@otaj
Copy link
Contributor Author

otaj commented Aug 3, 2022

Hi @nishantb06, thanks a lot, added you to the list!

@redleaf-kim redleaf-kim mentioned this issue Aug 3, 2022
Merged
14 tasks
@matsumotosan
Copy link
Contributor

Hi @otaj, do you mind adding me to the following:

  • pl_bolts.transforms.dataset_normalizations.*
  • pl_bolts.models.self_supervised.cpc.*

@otaj
Copy link
Contributor Author

otaj commented Oct 3, 2022

@Atharva-Phatak, @matsumotosan, thank you! You were added to the list! ⚡ 🔩

@lijm1358 lijm1358 mentioned this issue Oct 4, 2022
Merged
8 tasks
@senarvi
Copy link
Contributor

senarvi commented Oct 6, 2022

Would like to work on features around SimCLR

@Atharva-Phatak MoCo is very similar and could perhaps reuse some of the code. What do you think of refactoring them both and seeing if some of the code can be shared?

@Atharva-Phatak
Copy link
Contributor

@senarvi Yes believe that the code is same but if I am going to refactor both I will need some time. @otaj I understand that you have given us a deadline of two weeks would it be okay if took some more time ?

@senarvi
Copy link
Contributor

senarvi commented Oct 6, 2022

@senarvi Yes believe that the code is same but if I am going to refactor both I will need some time. @otaj I understand that you have given us a deadline of two weeks would it be okay if took some more time ?

@Atharva-Phatak let me know if I can help you. Or if you prefer, I can work on MoCo.

@Atharva-Phatak
Copy link
Contributor

@senarvi I believe if you could take over MoCo, at least for this iteration and in the next iteration, we can fully revise the SSL module as it is not much customizable :)

@matsumotosan matsumotosan mentioned this issue Oct 6, 2022
Merged
9 tasks
This was referenced Oct 6, 2022
Closed
Closed
This was referenced Oct 9, 2022
Closed
Draft
@Atharva-Phatak Atharva-Phatak mentioned this issue Oct 10, 2022
Merged
8 tasks
@otaj
Copy link
Contributor Author

otaj commented Oct 11, 2022

Hi, @senarvi, @Atharva-Phatak! I'm sorry for not being too responsive. Taking longer than two weeks is absolutely fine. I honestly don't have a strong preference regarding if you should review SSL module and MoCo together or separately, it seems you are capable of taking that decision into your hands ⚡

@Atharva-Phatak
Copy link
Contributor

Atharva-Phatak commented Oct 26, 2022 •
edited

I am taking these up.

pl_bolts.models.gans.dcgan.components.DCGANDiscriminator
pl_bolts.models.gans.dcgan.components.DCGANGenerator
pl_bolts.models.gans.dcgan.dcgan_module.cli_main
pl_bolts.models.gans.dcgan.dcgan_module.DCGAN

@Atharva-Phatak Atharva-Phatak mentioned this issue Oct 28, 2022
Merged
8 tasks
@senarvi senarvi mentioned this issue Oct 31, 2022
Merged
8 tasks
@senarvi
Copy link
Contributor

senarvi commented Oct 31, 2022

I opened a pull request for MoCo and opened some discussion there. Or what's the best place to discuss the details?

@Atharva-Phatak
Copy link
Contributor

I opened a pull request for MoCo and opened some discussion there. Or what's the best place to discuss the details?

Hi senarvi, since we are focusing on SSL methods mostly I created an issue #929 which highlights things that we need to refactor and we can continue the discussion from there onwards. I have also added few comments to your PR regarding the same.

@BaruchG BaruchG mentioned this issue Nov 2, 2022
Closed
8 tasks
@lijm1358
Copy link
Contributor

lijm1358 commented Nov 3, 2022

Hi, I'd like to work on pl_bolts.datamodules.cityscapes_datamodule.CityscapesDataModule!

@Atharva-Phatak
Copy link
Contributor

@lijm1358 Assigned :). Thanks :)

@otaj otaj mentioned this issue Nov 4, 2022
Closed
@Ce11an Ce11an mentioned this issue Nov 27, 2022
Merged
11 tasks
@lijm1358 lijm1358 mentioned this issue Dec 29, 2022
Draft
8 tasks
@stale
Copy link

stale bot commented Jan 7, 2023

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.

@stale stale bot added the won't fix This will not be worked on label Jan 7, 2023
@andrewaf1
Copy link

I would love to take on some of the RL stuff. Maybe pl_bolts.models.rl.dqn_model.DQN to start?

@stale stale bot removed the won't fix This will not be worked on label Jan 23, 2023
@Atharva-Phatak
Copy link
Contributor

@andrewaf1 Assigning that to you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers help wanted Extra attention is needed let's do it! Looking forward to have it implemented
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Continuation of #858 for #839 Cifar10 revision BaruchG/lightning-bolts