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

Make dependency of vissl optional #1276

Conversation

ar90n
Copy link
Contributor

@ar90n ar90n commented Apr 6, 2022

What does this PR do?

This PR is motivated to achieve the second change in this comment. And this PR contains the following modifications.

  • Make training_strategy, head, pretraining_transform in the arguments of __init__ of ImageEmbedder optional
  • Add DefaultAdapter to the Embedding task which is inspired by the Image Classification task's one.
  • Fix a test about integration with Fiftyone to work correctly
  • Add test cases for only embedding without VISSL

Part of #1031

This PR is based on #1271. Because its target branch was wrong.
I'm sorry for my wrong operations. I misunderstood that it would be okay to change its target branch.

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 🙃

@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #1276 (1dffc9d) into master (b88d1ba) will increase coverage by 0.00%.
The diff coverage is 92.45%.

@@           Coverage Diff           @@
##           master    #1276   +/-   ##
=======================================
  Coverage   91.55%   91.56%           
=======================================
  Files         284      285    +1     
  Lines       12729    12774   +45     
=======================================
+ Hits        11654    11696   +42     
- Misses       1075     1078    +3     
Flag Coverage Δ
unittests 91.56% <92.45%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
flash/image/embedding/strategies/default.py 88.88% <88.88%> (ø)
flash/core/utilities/imports.py 92.89% <100.00%> (+0.04%) ⬆️
flash/image/embedding/model.py 98.48% <100.00%> (+0.09%) ⬆️
flash/image/embedding/strategies/__init__.py 100.00% <100.00%> (ø)
...ash/image/embedding/strategies/vissl_strategies.py 100.00% <100.00%> (ø)
flash/text/question_answering/model.py 92.51% <0.00%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b88d1ba...1dffc9d. Read the comment docs.

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 😃 Just a couple of minor comments

CHANGELOG.md Outdated Show resolved Hide resolved
tests/image/embedding/test_model.py Outdated Show resolved Hide resolved
flash/image/embedding/strategies/default.py Outdated Show resolved Hide resolved
flash/image/embedding/strategies/default.py Outdated Show resolved Hide resolved
@ethanwharris ethanwharris added this to the 0.8.0 milestone Apr 6, 2022
@ethanwharris ethanwharris added the enhancement New feature or request label Apr 6, 2022
ar90n and others added 25 commits April 7, 2022 16:35
Co-authored-by: Ethan Harris <[email protected]>
Co-authored-by: Ethan Harris <[email protected]>
Co-authored-by: Ethan Harris <[email protected]>
@ar90n ar90n force-pushed the feature/make-dependency-of-vissl-optional branch from e8f0a40 to 705e8f2 Compare April 7, 2022 16:36
@ar90n ar90n changed the title Feature/make dependency of vissl optional Make dependency of vissl optional Apr 7, 2022
@ar90n ar90n marked this pull request as ready for review April 8, 2022 01:27
@ar90n
Copy link
Contributor Author

ar90n commented Apr 8, 2022

This PR is ready for review. Please review it.
I have one question about dealing with DeepSource. Could I ignore its failure as same as other PRs?

I'm sorry for making a messy commit log. I struggled to solve CI issues.

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.

LGTM 😃 @ar90n Fine to ignore the deep source issues, we plan to get rid of it. No worries about the commit history! That's why we always squash and merge 😆

@ethanwharris ethanwharris enabled auto-merge (squash) April 8, 2022 10:28
@ar90n
Copy link
Contributor Author

ar90n commented Apr 8, 2022

@ethanwharris
Thanks for your review. I'm so glad to make this PR accepted. I'm looking forward to this PR being merged.

@ethanwharris
Copy link
Collaborator

ethanwharris commented Apr 8, 2022

@ar90n No worries, thanks for your work on this!! Very glad to see it all working smoothly 😃

@ethanwharris ethanwharris merged commit f4f14b0 into Lightning-Universe:master Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants