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

Integration of lightning_utilties function into flash #1457

Merged
merged 23 commits into from
Oct 1, 2022

Conversation

uakarsh
Copy link
Contributor

@uakarsh uakarsh commented Sep 17, 2022

What does this PR do?

Fixes #1452

Currently, I was able to only find the changes in flash/core/utilities/imports.py

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 🙃

CC:
@carmocca @krshrimali

@krshrimali krshrimali self-assigned this Sep 18, 2022
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.

Thanks @uakarsh for the PR! 🚀 Left a small suggestion to keep the names as it is (coming from lightning_utilities). I'll just check tomorrow if we didn't miss anything, and once the comments are addressed, let's request @carmocca's review and merge it! :)

flash/core/utilities/imports.py Outdated Show resolved Hide resolved
flash/core/utilities/imports.py Outdated Show resolved Hide resolved
@krshrimali krshrimali added this to the 0.8.x milestone Sep 18, 2022
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.

I can usually commit to the branch, but I'm leaving these as suggestions as I would like if you also get to accept the changes I suggest. :) The flake8 needs some attention: https://github.com/Lightning-AI/lightning-flash/actions/runs/3072640704/jobs/4964256186, I suggested for the 2 that I could, but you'll have to remove find_spec import from flash/core/utilities/imports.py file as that import is also unused.

In case you don't have time, don't worry about it - let us know and I'll take it from here. 🔥

flash/core/utilities/imports.py Outdated Show resolved Hide resolved
flash/core/utilities/imports.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
Borda and others added 5 commits September 24, 2022 00:17
Co-authored-by: Kushashwa Ravi Shrimali <[email protected]>
1. Kept the import name of "module_available" and "compare_version" as it is
2. Removed the unused imports in the apply_func
3. "is_overridden" is used for "_is_overridden" in apply_func.py file
@codecov
Copy link

codecov bot commented Sep 24, 2022

Codecov Report

Base: 81.27% // Head: 92.33% // Increases project coverage by +11.06% 🎉

Coverage data is based on head (3c12949) compared to base (eb11c35).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #1457       +/-   ##
===========================================
+ Coverage   81.27%   92.33%   +11.06%     
===========================================
  Files         291      291               
  Lines       13197    13171       -26     
===========================================
+ Hits        10726    12162     +1436     
+ Misses       2471     1009     -1462     
Flag Coverage Δ
pytest 13.07% <85.07%> (-0.09%) ⬇️
tpu 13.07% <85.07%> (-0.09%) ⬇️
unittests 92.87% <100.00%> (+11.13%) ⬆️

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

Impacted Files Coverage Δ
flash/core/data/base_viz.py 100.00% <100.00%> (+4.34%) ⬆️
flash/core/utilities/apply_func.py 100.00% <100.00%> (+5.88%) ⬆️
flash/core/utilities/imports.py 94.07% <100.00%> (+2.60%) ⬆️
flash/image/detection/backbones.py 94.11% <100.00%> (+0.17%) ⬆️
flash/image/instance_segmentation/backbones.py 92.85% <100.00%> (+0.26%) ⬆️
flash/core/serve/dag/task.py 95.81% <0.00%> (-1.05%) ⬇️
flash/core/utilities/flash_cli.py 91.04% <0.00%> (+0.74%) ⬆️
flash/core/trainer.py 88.61% <0.00%> (+0.81%) ⬆️
flash/core/data/io/input_transform.py 91.34% <0.00%> (+0.96%) ⬆️
flash/core/data/utilities/classification.py 98.34% <0.00%> (+1.10%) ⬆️
... and 96 more

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Hey @uakarsh - Thank you so much! We are closing in, just left a comment to replace _is_overridden with is_overridden from lightning_utilities package. In case I wasn't clear enough in the review comment, or you have any queries, let me know - and I can push those changes for you.

Just one more step, and we are done. 🚀

flash/core/utilities/apply_func.py Outdated Show resolved Hide resolved
flash/core/utilities/imports.py Outdated Show resolved Hide resolved
flash/image/detection/backbones.py Outdated Show resolved Hide resolved
flash/image/instance_segmentation/backbones.py Outdated Show resolved Hide resolved
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.

Hi, @uakarsh - thank you so much for this PR! This LGTM, however I'll have to investigate the failure reported here, as it seems to be related to this PR. (not your fault at all, I'll take a look tomorrow)

Awesome work by the way, great job on keeping at it. 🚀 ⚡

flash/core/utilities/apply_func.py Outdated Show resolved Hide resolved
flash/core/utilities/apply_func.py Outdated Show resolved Hide resolved
@uakarsh
Copy link
Contributor Author

uakarsh commented Sep 25, 2022

Thanks for giving me this opportunity @krshrimali. I would be more than happy to solve more issues. I guess I am surely having fun trying to use the tools of Lightning AI, especially for experiments and Kaggle. Just a thought, currently in research, multimodality is something, which is used for obtaining good performance. So, how about introducing a multi-modal framework? I am not sure, how to go, but maybe something like using Image + the text inside the Image + Question for Answering. Do let me know your thoughts.

Copy link
Collaborator

@ethanwharris ethanwharris left a comment

Choose a reason for hiding this comment

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

Awesome!!! LGTM 😃

@ethanwharris ethanwharris enabled auto-merge (squash) October 1, 2022 10:00
@ethanwharris ethanwharris merged commit 41d1251 into Lightning-Universe:master Oct 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate lightning_utilities
5 participants