simplify model builders #5001
simplify model builders #5001
Conversation
💊 CI failures summary and remediationsAs of commit bc358bc (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
I think this is great and this is something we have discussed with @datumbox already, but I would prefer waiting a bit before doing it here: I'd like this to be a more pervasive change throughout torchvision, not just something specific to the models. Also, I'd like us to have a clearer deprecation policy, i.e. decide when we actually remove stuff. I want to get this done before then next release anyway, likely in January (release is planned for late Feb I think) Also, this is only tangentially related to this PR, so feel free not to address it here: it'd be great if we could advertise the new API in the deprecation message, and also help users with the transition. So ideally, the message could be something like
|
I can turn the positional to keyword only handling into a general decorator if you want. That way we can move forward with the model builders by using said decorator and handle all other cases later by simply using it.
In PyTorch core the "regular" policy is to deprecate for one release and unless there is major pushback remove it the next release.
Tentative release date is 04.03.2022.
@datumbox What would a good message look like? Please use |
There was a problem hiding this comment.
@pmeier I love the direction of this. I've added a few comments below, please have a look. Note that there is a final refactoring PR that is going to affect you at #5006. Hopefully we will merge it later today and avoid conflicts.
I would prefer waiting a bit before doing it here: I'd like this to be a more pervasive change throughout torchvision, not just something specific to the models.
I agree we should roll this out in the main area in a coordinated manner. Concerning the prototype area, I would be open adopting it early to avoid introducing changes over and over on the API. The Datasets API already uses it, so it makes sense to have it here as well. Thoughts?
I can turn the positional to keyword only handling into a general decorator if you want. That way we can move forward with the model builders by using said decorator and handle all other cases later by simply using it.
Sounds like a good idea. I'll love to see the details.
What would a good message look like?
I think the message provided by @NicolasHug looks great. The words pretrained, weights and ResnetWeights.Blahblah are the parameterized parts, which you already handle.
The new datasets make use of the positional argument, which is perfectly fine since they're new classes. But here we're deprecating exising functions and existing usage - I would prefer to do that in a few weeks when we have a good idea of what we want the deprecation process will look like. |
|
We had a chat with @datumbox . I'm fine with introducing the keyword-only deprecation here. That means that we need to get to an agreement regarding the keyword deprecations for the rest of the code base before we move the new models from prototypes to main. If we can't reach that point before the next release (unlikely, but possible), we would have to remove the deprecations. |
Agreed. I've captured this point step 1 of the release tasks at #4679 |
|
The warnings now look as follows:
Happy with them? |
|
I reviewed the proposed solution and looks great to me. I think we can implement the proposal on all model builders. |
| weights: Optional[DeepLabV3_ResNet50_Weights] = None, | ||
| progress: bool = True, | ||
| num_classes: Optional[int] = None, | ||
| aux_loss: Optional[bool] = None, | ||
| weights_backbone: Optional[ResNet50_Weights] = None, | ||
| **kwargs: Any, |
There was a problem hiding this comment.
This changes the signature of the method. Though a corner case, it means that now if someone passed an extra parameter as argument it won't be ignored but instead they will get an exception. Note that kwargs is currently supported by all model builders so I suggest leaving it in place.
There was a problem hiding this comment.
Though a corner case, it means that now if someone passed an extra parameter as argument it won't be ignored but instead they will get an exception.
That is kind of the point. The builder silently ignores wrong keyword arguments. Imagine you made a typo
deeplabv3_resnet50(weight=DeepLabV3_ResNet50_Weights.CocoWithVocLabels_V1)and instead of loading some weights, you get a randomly initialized model instead. I don't think this is expected neither a good thing to have. At the very least, we should emit a warning that there were unused keyword arguments.
Note that
kwargsis currently supported by all model builders so I suggest leaving it in place.
There are places where this is useful. For example the **kwargs of VGG variants like
vision/torchvision/prototype/models/vgg.py
Lines 172 to 179 in 93b26da
will ultimately be resolved in
vision/torchvision/models/vgg.py
Lines 35 to 38 in 93b26da
So if there is any unknown keyword argument, the class instantiation would fail.
There was a problem hiding this comment.
@pmeier I understand and I agree with many points you are raising here. My concern is that this is not the right PR to fix this. Fixing it here only resolves the problem for 2 models, leaving the rest affected. Shall we take this on a separate issue?
There was a problem hiding this comment.
@pmeier LGTM, thanks for the major readability improvement. The code looks much much better now.
I've pushed 3 commits, 1 for a missing verify call and 2 for kwargs. If you are OK with it, feel free to merge on green CI. Else revert and let's chat.
Summary: * simplify model builders * cleanup * refactor kwonly to pos or kw handling * put weight verification back * revert num categories checks * fix default weights * cleanup * remove manual parameter map * refactor decorator interface * address review comments * cleanup * refactor callable default * fix type annotation * process ungrouped models * cleanup * mroe cleanup * use decorator for detection models * add decorator for quantization models * add decorator for segmentation models * add decorator for video models * remove old helpers * fix resnet50 * Adding verification back on InceptionV3 * Add kwargs in DeeplabeV3 * Add kwargs on FCN * Fix typing on Deeplab * Fix typing on FCN Reviewed By: NicolasHug Differential Revision: D32950943 fbshipit-source-id: c2a5c21e48b7e6fffcbba2e1ee7ab9dd7dc4a306 Co-authored-by: Vasilis Vryniotis <[email protected]>
Addresses #4948.
This PR adds two decorators that help reducing verbose and duplicated code in the prototype model builders:
@handle_legacy_interfaceThis decorator does two things:pretrainedandweightsand warn thatpretrainedis deprecated.@handle_num_categories_mismatchThis decorator will raise an error ifnum_classesis passed, but doesn't match the number of categories of the pretrained weights.Examples:
cc @datumbox @bjuncek