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.

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

Add timm version check #1487

Merged
merged 8 commits into from
Feb 1, 2024
Merged

Add timm version check #1487

merged 8 commits into from
Feb 1, 2024

Conversation

guarin
Copy link
Contributor

@guarin guarin commented Jan 25, 2024

Changes

  • Add version check for timm
  • Only import AIM model parts if timm >= 0.9.9

This fixes import issues if users have an older version of timm installed. For details see:

@guarin
Copy link
Contributor Author

guarin commented Jan 25, 2024

FYI @adamjstewart. Would this fix the problem in torchgeo?

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (25894a9) 84.40% compared to head (7b458e8) 84.40%.

Files Patch % Lines
lightly/utils/dependency.py 75.00% 4 Missing ⚠️
lightly/models/utils.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1487      +/-   ##
==========================================
- Coverage   84.40%   84.40%   -0.01%     
==========================================
  Files         139      140       +1     
  Lines        5777     5783       +6     
==========================================
+ Hits         4876     4881       +5     
- Misses        901      902       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lightly/models/modules/__init__.py Outdated Show resolved Hide resolved
lightly/__init__.py Outdated Show resolved Hide resolved
lightly/__init__.py Outdated Show resolved Hide resolved
requirements/base.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

LGTM. The try-except is probably overkill if you want to simplify things. Thanks for addressing this so quickly!

Copy link
Contributor

@IgorSusmelj IgorSusmelj left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Everything looks good. I have a few suggestions:

requirements/minimal_requirements.txt Outdated Show resolved Hide resolved
@adamjstewart
Copy link
Contributor

Packaging is no longer needed now right?

@guarin
Copy link
Contributor Author

guarin commented Jan 26, 2024

Packaging is no longer needed now right?

Yes exactly :) Plus we actually verify whether the required classes are there instead of relying on the version.

The change should also speed up importing lightly in environments where torchvision and/or timm are installed as we now only import them when needed.

Copy link
Contributor

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

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

This works for me now! The edited test file passes and correctly skips certain tests because I have timm 0.9.2 installed.

tests/models/modules/test_masked_autoencoder.py Outdated Show resolved Hide resolved
Copy link
Contributor

@IgorSusmelj IgorSusmelj left a comment

Choose a reason for hiding this comment

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

This updated version is much more elegant and solves our need without new dependencies! Well done!

@guarin guarin merged commit 6e2ea0f into master Feb 1, 2024
11 of 12 checks passed
@guarin guarin deleted the add-timm-version-check branch February 1, 2024 08:27
@adamjstewart
Copy link
Contributor

Thanks for the quick fix @guarin, glad we'll be able to use future timm versions again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants