Fix: Add version check for timm to support mobilenetv5 models (fixes #39208)#39264
Fix: Add version check for timm to support mobilenetv5 models (fixes #39208)#39264VIGNESH15103 wants to merge 9 commits intohuggingface:mainfrom
Conversation
There was a problem hiding this comment.
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.
The version for mibilenetv5 should be timm >= 1.0.16
There was a problem hiding this comment.
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.
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.
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! |
qubvel
left a comment
There was a problem hiding this comment.
Thanks for iterating, a few more comments 🤗
| except RuntimeError as e: | ||
| raise ImportError( | ||
| f"You are trying to instantiate `{config.architecture}`, but it's not supported in timm={timm.__version__}. " | ||
| "Please try updating to the latest timm version with `pip install -U timm`." | ||
| ) from e |
There was a problem hiding this comment.
We should catch an exact exception that the model is unsupported, not a general one
| # 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.
do we still need this in image processor?
| try: | ||
| self.timm_model = timm.create_model( | ||
| config.architecture, pretrained=False, num_classes=config.num_labels, **extra_init_kwargs | ||
| ) | ||
| except RuntimeError as e: | ||
| raise ImportError( | ||
| f"You are trying to instantiate `{config.architecture}`, but it's not supported in timm=={timm.__version__}. " | ||
| "Please try updating to the latest timm version with `pip install -U timm`." | ||
| ) from e | ||
|
|
There was a problem hiding this comment.
maybe wrap this in a function
|
[For maintainers] Suggested jobs to run (before merge) run-slow: timm_wrapper |
…remove redundant checks
6afd22c to
5bfcdab
Compare
|
Hi! I've addressed all your feedback:
Could you please take another look and let me know if there's anything else to improve? If everything looks good, it would be awesome if you could approve and merge it 😊 |
qubvel
left a comment
There was a problem hiding this comment.
Hey! I guess you are using some AI model to create PR, please review changes before submitting them 🤗
| # mobilenetv5 models require timm >= 0.9.10 | ||
|
|
| try: | ||
| self.timm_model = safe_create_model(config.architecture, 0, **extra_init_kwargs) | ||
|
|
||
| except RuntimeError as e: | ||
| raise ImportError( | ||
| f"You are trying to instantiate `{config.architecture}`, but it's not supported in timm={timm.__version__}. " | ||
| "Please try updating to the latest timm version with `pip install -U timm`." | ||
| ) from e |
There was a problem hiding this comment.
we should just call for safe_create_model with named args, no need for try catch anymore
| def safe_create_model(architecture, num_classes, **kwargs): | ||
| try: | ||
| return timm.create_model( | ||
| architecture, | ||
| pretrained=False, | ||
| num_classes=num_classes, | ||
| **kwargs, | ||
| ) | ||
| except RuntimeError as e: | ||
| raise ImportError( | ||
| f"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`." | ||
| ) from e |
There was a problem hiding this comment.
we should check for a specific message in the exception, not just catch RuntimeError
What does this PR do?
Fixes issue #39208: Adds a version check for
timmto ensure support for models likemobilenetv5_300m_enc, which requiretimm >= 0.9.10.Motivation
The issue occurred because the
mobilenetv5_300m_encmodel name was not recognized in older versions oftimm. This PR adds a conditional check to raise an informativeImportErrorif an unsupported version is detected.Changes
packaging.versioninsideTimmWrapperImageProcessor.__init__.mobilenetv5model is used withtimm < 0.9.10.Issue Link
Closes #39208
Reviewer Suggestion
@amyeroberts @qubvel (vision models)