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

Multi pretrained weights: Cleanups and Refactoring #4652

Closed
5 tasks done
jdsgomes opened this issue Oct 19, 2021 · 6 comments · Fixed by #4830, #4849, #4854, #4876 or #4940
Closed
5 tasks done

Multi pretrained weights: Cleanups and Refactoring #4652

jdsgomes opened this issue Oct 19, 2021 · 6 comments · Fixed by #4830, #4849, #4854, #4876 or #4940

Comments

@jdsgomes
Copy link
Contributor

jdsgomes commented Oct 19, 2021

Candidates:

  • Reduce code duplication on model builders by refactoring. For example potentially push the weigh loading mechanism in the private builder methods currently inherited from torchvision.models. Also we could provide a pretrained deprecation method.
  • Review recipe links and ensure they are accurate for all Community Contributed models. Check if possible for some of them to remap their training process to our current ref scripts.
  • Decide how quantized models will link to their unquantized versions.
  • Naming conventions:
    • The name of the Weights base Enum class and the naming convention of the inheriting classes. Also make sure that all classes are aligned with the model builder names. How are we going to handle _ in names (for example on inception_v3 and the resnext models)?
    • The name of the WeightEntry data class.
    • The name of Weights.verify() method (because it does more than just verification).
    • The names of the enum values; also their styles (all capital?). Example: ImageNet1K_RefV1
    • Currently the _RefV1 of one model doesn't have to be _RefV1 of another model. For example ResNet vs RegNets use different recipes with the same enum value. In other cases, we skip _RefV1 and go directly to _RefV2, for example the WideResNet50 weights. Is this a problem?
  • Decide the signature of models._api.get_weight() and make it public. Ensure it's torchhub-friendly.

cc @datumbox @pmeier @bjuncek @NicolasHug

@jdsgomes
Copy link
Contributor Author

Just opening the discussion here regarding the naming for Weights and WeightEntry. As This classes store metadata about weights and how they were generated (recipe + transforms) rather than the actual weights so I think we could try to find a better name.

Having said that I'm struggling to find a much better name, but want to leave some suggestions for discussion:

  • PretrainedModelEnum and PretrainedModel
  • PretrainedModel and PretrainedModelEntry
  • Weights and WeightsInfo

Open for more suggestions. Naming IS hard :)

@jdsgomes
Copy link
Contributor Author

Inside the Weights class we have a verify method that has some side effects depending on the input parameter. I suggest to change this to build or create it would be clear that we are trying to build an object from a string or from another object of the same type (in this case it will be an identity function). And this builder would throw an exception if the arguments were invalid.

@datumbox
Copy link
Contributor

datumbox commented Oct 19, 2021

Concerning the WeightEntry name, Motto has advocated using the word Bundle. You can read more here: datumbox/dapi-model-versioning#1 (comment)

@NicolasHug
Copy link
Member

Thanks for opening this discussion @jdsgomes , I agree naming is important! (and hard)

In this case, everything is private right? If we don't expect users to interact directly with these classes (i.e they won't be inheriting from them), I feel like there isn't too much risk in choosing an imperfect naming, because this will only really affect us (the devs).

I personally don't mind Weights and WeightEntry, they're not super obvious at first but it becomes clear when one sees the instanciation of e.g. the ResNet50Weights class. I think this will be the case for any naming we choose IMHO. My only note about the current naming is that WeightEntry could probably be WeightsEntry.

I think your suggestions above are good too. I'll just add another one: WeightsEnum and Weights. e.g.:

class ResNet50Weights(WeightsEnum):
    ImageNet1K_RefV1 = Weights(...)
    ImageNet1K_RefV2 = Weights(...)
    ...

@datumbox
Copy link
Contributor

@NicolasHug Thanks for the thoughts. Another idea we discussed with @jdsgomes offline was the fact that the names of Enums are too long due to our naming conventions for some models. For example for fasterrcnn_resnet50_fpn() the enum is FasterRCNNResNet50FPNWeights which frankly is too long and too unreadable. One option would be to drop completly the "Weights" from the inheriting classes. For example: FasterRCNNResNet50FPN.

At any case, please everyone feel free to add candidates on the list. To unblock the multi-pretrained initiative we were lenient on the reviews concerning nitpicking, naming etc, so we should definitely do a more careful round before moving the code to main torchvision. :)

@datumbox datumbox changed the title Cleanups, review names and do nitpicking Multi pretrained weights: Cleanups, review names and do nitpicking Oct 20, 2021
@datumbox datumbox changed the title Multi pretrained weights: Cleanups, review names and do nitpicking Multi pretrained weights: Cleanups and Refactoring Oct 28, 2021
@datumbox datumbox linked a pull request Nov 2, 2021 that will close this issue
@datumbox datumbox linked a pull request Nov 3, 2021 that will close this issue
@datumbox datumbox linked a pull request Nov 4, 2021 that will close this issue
@datumbox datumbox linked a pull request Nov 15, 2021 that will close this issue
@datumbox datumbox linked a pull request Nov 16, 2021 that will close this issue
6 tasks
@datumbox
Copy link
Contributor

I had an offline discussion with @NicolasHug and we discussed the following optimizations:

  1. For the Weights classes, keep using necessary capitalzation to help readability such as ResNeXt to be aligned with bibliography but ensure this is consistent across the codebase (for example VisionTransformer_B_32Weights => ViT_B_32Weights)
  2. Use underscore where they are on the builder. For example InceptionV3Weights => Inception_V3Weights
  3. Rename classes Weights => WeightsEnum, WeightEntry => Weights
  4. Rename values ImageNet1K_Community => ImageNet1K_V1, ImageNet1K_RefV2 => ImageNet1K_V2
  5. Make get_weight work using the fully qualified name and expose it publicly get_weight and filter it from tests
  6. Implement the default mechanism using Enum aliases (default = ImageNet1K_RefV2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment