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.

Sign up for GitHub

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

Jump to bottom

Add missing handle_legacy_interface() calls #6565

Merged
merged 5 commits into from
Sep 12, 2022
Merged

Add missing handle_legacy_interface() calls #6565

merged 5 commits into from
Sep 12, 2022

Conversation

datumbox
Copy link
Contributor

Fixing #6564

datumbox
datumbox commented Sep 12, 2022
View reviewed changes
test/test_extended_models.py Outdated Show resolved Hide resolved
torchvision/models/regnet.py Outdated Show resolved Hide resolved
torchvision/models/vision_transformer.py Outdated Show resolved Hide resolved
YosuaMichael
YosuaMichael reviewed Sep 12, 2022
View reviewed changes
Copy link
Contributor

@YosuaMichael YosuaMichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @datumbox thanks for the PR, overall it looks good. I notice there is a failure on the test, is this related to the changes?

torchvision/models/regnet.py Outdated Show resolved Hide resolved
test/test_extended_models.py Outdated Show resolved Hide resolved
@datumbox
Copy link
Contributor Author

datumbox commented Sep 12, 2022 •
edited
Loading

Thanks for the feedback @YosuaMichael. I'll make some changes shortly.

Concerning the failing tests, they don't seem related. I'll open a ticket with RelEng in a bit.

@NicolasHug
Copy link
Member

FYI unless we are planning on a 0.13.2 bugfix release (I'm not sure it will happen), the deprecation message will be inaccurate as it will mention that pretrained=True was deprecated in 0.13, but in reality this will only start to to be available (and deprecated) in 0.14.

jdsgomes
jdsgomes approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@jdsgomes jdsgomes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks

YosuaMichael
YosuaMichael approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@YosuaMichael YosuaMichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@datumbox datumbox mentioned this pull request Sep 12, 2022
Closed
@datumbox datumbox merged commit a89b195 into pytorch:main Sep 12, 2022
@datumbox datumbox deleted the models/add_compatibility branch September 12, 2022 17:03
facebook-github-bot pushed a commit that referenced this pull request Sep 12, 2022
@datumbox @facebook-github-bot
[fbsync] Add missing handle_legacy_interface() calls (#6565)
054a1a0 
Summary:
* Add `handle_legacy_interface()` to all new models.

* Fix imports

* Addressing review comments.

* Fix linter

* Addressing further comments.

Reviewed By: jdsgomes, YosuaMichael, atalman

Differential Revision: D39435623

fbshipit-source-id: ee260f3b85e15039427a492e352f95a5d9b9728d
@NicolasHug
Copy link
Member

NicolasHug commented Sep 13, 2022 •
edited
Loading

Hi all,

Any feedback / follow-up on #6565 (comment) ?

@YosuaMichael
Copy link
Contributor

@NicolasHug sorry just noticed your comment, if I understand correctly the multi-weight API is (where we also deprecate pretrained=True) is released on v0.13: https://github.com/pytorch/vision/releases. So I think it should the handle_legacy text should be correct from v0.13 in general.

However I agree that with this PR, some model that just got the decorator handle_legacy_interface() should not be deprecated on v0.13.

In my opinion, maybe we should bump the deprecation version for all model using handle_legacy_interface to v0.14.

WDYT @NicolasHug @datumbox @jdsgomes ?

@NicolasHug
Copy link
Member

if I understand correctly the multi-weight API is (where we also deprecate pretrained=True) is released on v0.13: https://github.com/pytorch/vision/releases. So I think it should the handle_legacy text should be correct from v0.13 in general.

Yes you understand correctly - the deprecation message is correct for most models, but not for the models tackled by this PR.

As mentioned in #6564, I think we should have a team discussion about whether it makes sense to postpone the removal of a deprecated API (I added an item for the next team meeting. This somewhat relates to this thread as well #6258 (comment)).

As far as the deprecation message for this PR's models though, a simple fix would be to change "deprecated in 0.13" to "deprecated in 0.14" (we can keep the planned removal for 0.15 IMHO, since this is a fairly exceptional case).

@YosuaMichael
Copy link
Contributor

@NicolasHug Agree with changing to deprecated in 0.14 but in my opinion we should move back the planned removal to 0.16 just for consistency. But not really a strong opinion.

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

@jdsgomes jdsgomes jdsgomes approved these changes

@YosuaMichael YosuaMichael YosuaMichael approved these changes

Assignees
No one assigned
Labels
bug cla signed module: models
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@datumbox @NicolasHug @YosuaMichael @jdsgomes @facebook-github-bot