-
Notifications
You must be signed in to change notification settings - Fork 301
Add MobileNetV5 to KerasHub #2399
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: master
Are you sure you want to change the base?
Conversation
Let's start the review rounds now that the model numerics match within a 1e-5 tolerance and the design integrates with Gemma 3n!! |
/gemini review |
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.
Code Review
This pull request introduces MobileNetV5 to KerasHub, following the established modular structure with a backbone, classifier, preprocessor, tests, and a checkpoint conversion script. The implementation is comprehensive and well-organized. My primary feedback revolves around ensuring model serializability. Several new custom layers are missing the get_config
method, which is critical for saving and loading models. I have detailed these instances in my comments. Additionally, I've identified a couple of latent bugs in the MobileNetV5Builder
concerning the handling of the 'ir' block type, which, while not affecting current presets, should be addressed for future robustness. Overall, this is a solid contribution, and addressing the serialization issues is the main priority.
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.
Thank you for the contribution, I have added my comment.
Can you add the test smallest preset test, you can mark as skip test for now.
Also, add on the fly conversion from hf here https://github.com/keras-team/keras-hub/tree/master/keras_hub/src/utils/timm
@sachinprasadhs Resolved all comments; thanks!
Since only one preset is currently exposed for MobileNetV5, are these two additions really needed? It might make sense to keep them if other presets are later added by TIMM / PyTorch Image Models, but for now, I’m not too sure; since all presets == the smallest preset, and there’s only one preset to load from HF, which we’re already providing with our Kaggle preset. Would love your suggestions! |
Usually for the models ported from HF, we require the on the fly conversion as well so that the model can be loaded from Kaggle as well as HF. |
Makes sense, will cover the same! |
keras_hub/src/models/mobilenetv5/mobilenetv5_image_classifier.py
Outdated
Show resolved
Hide resolved
The same still holds true. |
Thanks, overall it looks good to me. |
As far as I understand, exposing the complete task model allows us to bundle the ported preprocessing with it as well. Would love to know your thoughts, though. |
The current design integrates well within the Gemma 3n scope. Ready for reviews!
Numerics Matching (Precision: 1e-5)
cc: @divyashreepathihalli @mattdangerw
Colab Notebook
Numerics Validation Reproducibility Notebook
Closes #2401.