Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Updated the learn2learn "image_classification_imagenette_mini" example #1383

Merged
merged 13 commits into from
Aug 26, 2022

Conversation

uakarsh
Copy link
Contributor

@uakarsh uakarsh commented Jul 6, 2022

What does this PR do?

There were some problems in the downloading of the dataset as well as defining the ImageClassificationInputTransform, for the ImageClassificationData, so I have remade the tutorial for easy integration of learn2learn with flash
Fixes #1376

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?

Definitely
Make sure you had fun coding 🙃

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #1383 (5897183) into master (0e21259) will increase coverage by 2.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1383      +/-   ##
==========================================
+ Coverage   90.38%   92.90%   +2.51%     
==========================================
  Files         286      286              
  Lines       12861    12861              
==========================================
+ Hits        11625    11949     +324     
+ Misses       1236      912     -324     
Flag Coverage Δ
unittests 92.90% <ø> (+2.51%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flash/text/question_answering/model.py 93.19% <0.00%> (-1.37%) ⬇️
flash/core/classification.py 95.15% <0.00%> (+0.60%) ⬆️
flash/core/adapter.py 97.14% <0.00%> (+1.42%) ⬆️
flash/core/data/utilities/loading.py 98.52% <0.00%> (+1.47%) ⬆️
flash/core/data/utilities/paths.py 92.53% <0.00%> (+1.49%) ⬆️
flash/core/data/data_module.py 95.27% <0.00%> (+1.71%) ⬆️
flash/image/classification/model.py 81.03% <0.00%> (+1.72%) ⬆️
flash/image/data.py 100.00% <0.00%> (+3.50%) ⬆️
flash/core/model.py 91.42% <0.00%> (+3.57%) ⬆️
flash/image/segmentation/heads.py 95.65% <0.00%> (+4.34%) ⬆️
... and 28 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@krshrimali
Copy link
Contributor

Hi, @uakarsh - Thank you for the PR, I appreciate you taking a look at it. Apologies for the delay in review.

Would you mind converting this into a script instead? We usually prefer having python scripts in flash_examples, and any notebooks currently go in flash_notebooks folder. This will also help us review the script, and add suggestions.

A few points:

  1. I guess you changed the batch sizes to 1 for testing your PR, so can you revert those parameters to the original? (unless there was another reason that I'm missing)
  2. Any reason you changed the tthe ransforms applied for training stage by adding train_per_sample_transform? Just curious, I noticed that we didn't have those before (please correct me if I'm wrong).
  3. I see that you have gotten rid of ddp_shared accelerator which I think makes sense for examples. cc: @ethanwharris - I think it's a good idea to not have multi-gpu enabled by default in examples?

Also, just FYI:

We made a few changes to the transform API, if I'm not wrong, it now takes transform= ImageClassificationInputTransform in your ImageClassificationData.from_tensors call (instead of train_transform and val_transform kwargs). Whether you need to make this change in this PR or not, depends on which release this PR land for. I think @ethanwharris can comment more on this. :)

@ethanwharris ethanwharris added this to the 0.8.0 milestone Jul 11, 2022
@uakarsh
Copy link
Contributor Author

uakarsh commented Jul 12, 2022

Sure, I can convert it into a script

For your questions:

  1. The reason for using batch size 1 is, that I was using Amazon Sagemaker Studio (and, taking a batch size of more than 1, would lead to CUDA OUT OF MEMORY ERROR, so I had no other option but to use a batch size of 1 for making the code run

  2. It was taken from here

  3. I removed it because I think it is not useful for people having only a single GPU (and I rely only on Colab/Kaggle/Sagemaker Studio, so for that purpose I removed the DDP strategy) for coding/ removing the bugs

Definitely, it depends upon Flash's version of download, but when I just did pip install, it threw an error of train_transform and validation_transform, but in the Github Repo, it was just transform, maybe the main branch is not deployed yet, correct me if I am wrong

@ethanwharris
Copy link
Collaborator

Hey @uakarsh instead of creating a script from scratch, could you just apply your changes to the script here: https://github.com/Lightning-AI/lightning-flash/blob/master/flash_examples/integrations/learn2learn/image_classification_imagenette_mini.py

Happy for you to drop the ddp_shared, looks like a typo anyway as I guess it was meant to be ddp_sharded, not too sure.

Hope that helps 😃

@krshrimali
Copy link
Contributor

Hey, @uakarsh - Any updates on this? :) Let us know if you need any help.

@uakarsh uakarsh requested a review from krshrimali as a code owner July 28, 2022 06:40
@uakarsh
Copy link
Contributor Author

uakarsh commented Jul 28, 2022

Would surely do it today. Got a bit engaged in my own work and about using PyTorch Lightning with Speech Recognition.

@uakarsh
Copy link
Contributor Author

uakarsh commented Jul 28, 2022

Update: Have updated the script (along with instructions for downloading the dataset) and removed the notebook.

Copy link
Contributor

@krshrimali krshrimali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice attention to detail, @uakarsh - thanks for fixing the example script. We appreciate it!

I'm only wondering if this will fail the CI since we expect the user to download the dataset. I'll keep an eye on the CI, and add a custom download function until learnables/learn2learn#310 is fixed.

Also, can you add an entry to the CHANGELOG? If you are occupied today, I can do it once the CI is green.

Thanks! 🎉

@uakarsh
Copy link
Contributor Author

uakarsh commented Jul 28, 2022

Sure, I have added the entry for the updation in CHANGELOG.md The change is here Is there any need of creating a PR for the same updation, i.e in CHANGELOG.md ?

@krshrimali krshrimali enabled auto-merge (squash) August 2, 2022 05:45
@krshrimali krshrimali added the bug / fix Something isn't working label Aug 22, 2022
@krshrimali
Copy link
Contributor

krshrimali commented Aug 26, 2022

Failing tests are irrelevant to this PR. cc: @ethanwharris - can you please merge this whenever you are back?

@uakarsh - Apologies for the delay in getting this merged. The CI was failing for a couple of weeks, and we had to prioritize fixing it! As always, we appreciate your contribution. 🎉

Also, the CHANGELOG entry can just go in this PR! I've pushed the entry from your commit to this branch, hope that's fine! :)

@krshrimali krshrimali merged commit 6859fa1 into Lightning-Universe:master Aug 26, 2022
@uakarsh
Copy link
Contributor Author

uakarsh commented Aug 26, 2022

Hi @krshrimali, thanks for merging this. This is my first contribution to any issue on Github, so thanks for giving me this chance and looking forward to helping/resolving more issues.

@krshrimali
Copy link
Contributor

Hi @krshrimali, thanks for merging this. This is my first contribution to any issue on Github, so thanks for giving me this chance and looking forward to helping/resolving more issues.

It's always a memory, the first contribution! I still remember the exact time when I got my first PR merged, wish you the best! If you ever find an issue you find exciting to work upon, feel free to ping me, and I'll be happy to help wherever I can 🎉 ⚡

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug / fix Something isn't working
Projects
None yet
3 participants