-
Notifications
You must be signed in to change notification settings - Fork 29.6k
Fix: Add version check for timm to support mobilenetv5 models (fixes #39208) #39264
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
base: main
Are you sure you want to change the base?
Fix: Add version check for timm to support mobilenetv5 models (fixes #39208) #39264
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @VIGNESH15103, thanks a lot for the fix, I would suggest we add a check for config loading, not for the image processor
# mobilenetv5 models require timm >= 0.9.10 | ||
if architecture is not None and architecture.startswith("mobilenetv5"): | ||
if version.parse(timm.__version__) < version.parse("0.9.10"): | ||
raise ImportError( | ||
f"`mobilenetv5` models require `timm >= 0.9.10`, but found {timm.__version__}. " | ||
"Please upgrade `timm` to the latest version." | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version for mibilenetv5 should be timm >= 1.0.16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @qubvel! Just to confirm — would it be appropriate to add the version check inside TimmWrapperModel.init and TimmWrapperForImageClassification.init right before timm.create_model(...) based on config.architecture? I’ll update accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sound's good, but let's make it a bit more general, let's wrap timm.create_model
to catch the RuntimeError
and suggest to update timm
with clear message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smt like
You are trying to instantiate `{architecture}`, but it's not supported in timm={timm.__version__}. Please, try updating to the latest timm version with `pip install -U timm`.
50ae8c5
to
22a2cee
Compare
Following the feedback:
Kindly requesting a review and workflow approval when convenient. Thanks a lot! |
[For maintainers] Suggested jobs to run (before merge) run-slow: timm_wrapper |
What does this PR do?
Fixes issue #39208: Adds a version check for
timm
to ensure support for models likemobilenetv5_300m_enc
, which requiretimm >= 0.9.10
.Motivation
The issue occurred because the
mobilenetv5_300m_enc
model name was not recognized in older versions oftimm
. This PR adds a conditional check to raise an informativeImportError
if an unsupported version is detected.Changes
packaging.version
insideTimmWrapperImageProcessor.__init__
.mobilenetv5
model is used withtimm < 0.9.10
.Issue Link
Closes #39208
Reviewer Suggestion
@amyeroberts @qubvel (vision models)