Remove deprecated APIs for 0.14 #6258

NicolasHug · 2022-07-11T13:18:29Z

git grep -n "0\.14" torchvision/ now gives zero output

…l keep them around?

NicolasHug · 2022-07-11T13:22:26Z

test/test_datasets_download.py



 def voc():
+    # TODO: Also test the "2007-test" key


I'm conveniently leaving this as a TODO, because we actually don't try to download voc() anyway: further down in this file, the call to voc() is commented out because their server is unstable.

NicolasHug · 2022-07-11T13:24:41Z

torchvision/csrc/models/modelsimpl.h

+      "The vision::models namespace is not actively maintained, use at "
+      "your own discretion. We recommend using Torch Script instead: "


Looks like we originally didn't intend to deprecate the C++ models: at least according to #4375 (review), the original intention was to stop maintaininng them but to still keep them around.

So I put back the original warning, instead of removing the files. I'll still wait for @datumbox 's input on this tho.

Thanks for checking @NicolasHug. The intention was to remove them. The majority of the new models can't be added on C++ because the layers are only available on Python. So I think we are good to drop them, which was the intention when we deprecated them.

…ed_0_14

… into remove_deprecated_0_14

NicolasHug · 2022-07-28T09:50:07Z

@datumbox following up on #6258 (comment) about the C++ models: if that works for you, I'll remove them in another PR. The only reason is that I fear that removing them might break some stuff internally. I'd rather isolate that in a separate PR, to avoid having to potentially revert other unrelated changes (like the rest of the deprecations).

jdsgomes · 2022-08-03T16:46:45Z

@kazhang thanks for pointing out that _transforms_video.py is still referred internally so we need to discuss a plan for deprecating this particular file.

NicolasHug · 2022-08-03T17:06:44Z

Thanks @jdsgomes . I opened D38393214 to see what tests might break if we removed these files.

datumbox · 2022-08-04T09:32:48Z

@NicolasHug I don't have a super strong opinion about splitting the PRs if that simplifies your work. Curious why you think this can cause issues on FBcode though. As far as I see we don't even compile these sources on BUCK, so I don't see how people are using them. Have you saw anything different? If not, you can add the removal here; otherwise send a separate PR that removes the endpoints in question as you proposed.

Happy to provide the necessary stamps once you confirm things work on FBcode + the tests pass.

datumbox · 2022-09-22T11:21:41Z

torchvision/transforms/_functional_video.py

-
-
-warnings.warn(
-    "The 'torchvision.transforms._functional_video' module is deprecated since 0.12 and will be removed in 0.14. "


I commented this on the internal diff that @YosuaMichael opened to check the deprecation but for visibility to the community here is my comment here:

we can not deprecate torchvision.transforms._transforms_video since we don't offer currently an alternative. I will be working on this next week. I recommend changing the deprecation message on OSS to notify that we would be removing this on a future version and then go from scratch through a deprecation process once we have a clear alternative to define.

@datumbox I add back this file and change the warnings message, could you double check if this message is good? We can change the message to give exact version when it will be removed once we have a clear alternative as you said.

I was wondering if it's worth making this change on a separate PR on main because this one is quite massive. The message looks OK to me. There are a few more things we would need to revert such as the unit-tests for the specific module.

I'm not sure we really need to delay the removal here: on top of being deprecated, these APIs are private. Users who are still relying on it have been ignoring strong signals. IMHO we should either:

contact all downstream users who are still relying on this and tell them they have 10 days (or whatever) to update their code

be nice to them and submit diffs so that they vendor _functional_video.py themselves.

For the unit-test, do you mean the test_transforms_video.py? I also reverted it in this PR so it is not deleted.
For changing the message, I think it is a good idea to have separate PR just to make sure it has more visibility for the change in deprecation plan. I will revert back the message here and create another PR for that then.

@NicolasHug in my understanding, currently there is no alternatives for the _functional_video.py. In this situation, it will be very difficult for the users to update their code by just changing API or import.

By they vendor _functional_video.py themselves do you mean to copy and paste the _function_video.py on users code?

@NicolasHug Multiple internal users are still depending on this. Unless you want to dedicate your next 10 days porting their code away from these definitions and working with each individual team to ensure the results in production are identical, I don't think we can proceed with the removal.

If you check Yosua's Diff you will see multiple tests failing. In addition to the internal stuff, libraries such as PyTorch Video (cc @haooooooqi @lyttonhao) and ClassyVision (cc @jdsgomes @nairbv) are also affected.

IMO the issue with the specific deprecation is that it doesn't provide an actionable alternative that works out of the box for users and that lead to them not moving away from it. Though I agree there are concerns about respecting our own policies, we probably want to avoid breaking all downstream projects. I recommend reverting this removal to unblock the rest of the deprecations that are more straightforward and bring this topic up for discussion on the next weekly. There we can decide if we should move forwards with deprecating the specific part or not, without delaying the rest of the work.

By they vendor _functional_video.py themselves do you mean to copy and paste the _function_video.py on users code?

Yes, I mean that each downstream library could simply copy this file in their code-base. _functional_video.py is a small self-contained file that only has torch as dependency, so this would be a very reasonable ask from us. (the same goes for _transforms_video.py which just depends on torch/torchvision)

I recommend reverting this removal to unblock the rest of the deprecations that are more straightforward and bring this topic up for discussion on the next weekly. There we can decide if we should move forwards with deprecating the specific part or not, without delaying the rest of the work

I'm not sure I understand what we mean by "move forwards with deprecating the specific part or not", but I agree we can discuss how to move forward with _functional_video.py case separately (urgently, considering the approaching release).

@datumbox yes I confirm that _ClassyVision uses _transforms_video.py. I also see (less) references for _functional_video.py, so @NicolasHug do dpwnstream this file could be feasible

If I understand correctly, we are agree to discuss how to handle _functional_video.py (probably in weekly). And if we decide to remove it, I will create separate PR for that.

…on separate PR

YosuaMichael · 2022-09-26T13:13:42Z

@NicolasHug @datumbox @jdsgomes Could you check if this PR is okay to merge?
I think regarding the removal of deprecated API, other than this PR only the following two things left:

Removal of cpp models: Remove cpp model in v0.14 due to deprecation #6632
To be discussed whether or not we remove transforms/_functional_video.py and transforms/_transforms_video.py but we agree this will be on separate PR

I have checked and everything seems fine to me on this PR and I prefer if we can merge this PR faster and do fbsync in case anything we need to revert.

NicolasHug

GH doesn't let me approve as I'm the original author, but LGMT. Thanks @YosuaMichael

datumbox

@YosuaMichael stamping to unblock. LGTM, with only 2 questions below.

GH doesn't let me approve as I'm the original author,

@NicolasHug do you prefer to do this on a new PR so that the attribution is right?

torchvision/models/mobilenetv2.py

torchvision/models/mobilenetv3.py

NicolasHug · 2022-09-26T13:39:01Z

@NicolasHug do you prefer to do this on a new PR so that the attribution is right?

Just FYI, the changes in this PR come from my commits.

But whether this PR gets merged or another makes no difference to me.

YosuaMichael · 2022-09-27T13:04:36Z

Just to update, I found 2 breakages internally after trying this changes. I have created diffs to fix for each of them, and I will merge this PR once the fix is landed.

Break due to Kinetics400 removal: D39832659
Break due removal of resample and fillcolor argument on RandomAffine: D39843275

YosuaMichael · 2022-09-28T13:11:59Z

The failed test seems to be not related to this PR, will merge

github-actions · 2022-09-28T13:12:32Z

Hey @YosuaMichael!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

Summary: * Remove Kinetics400 class * Remove '2007-test' in VOC * Remove some MobileNet layer classes * Remove torchvision/models/segmentation/segmentation.py * Remove some MultiScaleRoIAlign methods * Remove torchvision/transforms/_functional_video.py * Remove torchvision/transforms/_transforms_video.py * Remove resample parameter in transforms * Remove 'range' parameter * Remove 'fill_value' parameter in transforms * Revert to original warning for C++ models - looks like we should still keep them around? * pre-commit * Fix docs * Remove test/test_transforms_video.py * Some fixes * Remove more tests * Revert changes to C++ models * Add back _transforms_video and change warning message * Change back the warning message, and will change the warning message on separate PR Reviewed By: YosuaMichael Differential Revision: D39886047 fbshipit-source-id: 52e3629ce0342817b6746351da6025cbc8a9589d Co-authored-by: YosuaMichael <yosuamichaelm@gmail.com> Co-authored-by: Yosua Michael Maranatha <yosuamichael@fb.com>

Summary: * Remove Kinetics400 class * Remove '2007-test' in VOC * Remove some MobileNet layer classes * Remove torchvision/models/segmentation/segmentation.py * Remove some MultiScaleRoIAlign methods * Remove torchvision/transforms/_functional_video.py * Remove torchvision/transforms/_transforms_video.py * Remove resample parameter in transforms * Remove 'range' parameter * Remove 'fill_value' parameter in transforms * Revert to original warning for C++ models - looks like we should still keep them around? * pre-commit * Fix docs * Remove test/test_transforms_video.py * Some fixes * Remove more tests * Revert changes to C++ models * Add back _transforms_video and change warning message * Change back the warning message, and will change the warning message on separate PR Reviewed By: datumbox Differential Revision: D40015852 fbshipit-source-id: 45e31f6ea00c25a4d4db719e8856784cb40ddfd8 Co-authored-by: YosuaMichael <yosuamichaelm@gmail.com> Co-authored-by: Yosua Michael Maranatha <yosuamichael@fb.com>

NicolasHug added 11 commits July 11, 2022 13:24

Remove Kinetics400 class

fcb22e9

Remove '2007-test' in VOC

37a437d

Remove some MobileNet layer classes

57915a5

Remove torchvision/models/segmentation/segmentation.py

54abb31

Remove some MultiScaleRoIAlign methods

4796dca

Remove torchvision/transforms/_functional_video.py

9e4d225

Remove torchvision/transforms/_transforms_video.py

5498794

Remove resample parameter in transforms

d641a69

Remove 'range' parameter

8bd7e73

Remove 'fill_value' parameter in transforms

c87f0a0

Revert to original warning for C++ models - looks like we should stil…

f3c8f09

…l keep them around?

facebook-github-bot added the cla signed label Jul 11, 2022

pre-commit

28964a2

NicolasHug commented Jul 11, 2022

View reviewed changes

NicolasHug added 8 commits July 12, 2022 13:38

Fix docs

98e912a

Remove test/test_transforms_video.py

12dddf9

Some fixes

96dde42

Remove more tests

e06f5a8

Merge branch 'main' into remove_deprecated_0_14

c026efc

Revert changes to C++ models

63b1ea7

Merge branch 'main' of github.com:pytorch/vision into remove_deprecat…

0b6bcdb

…ed_0_14

Merge branch 'remove_deprecated_0_14' of github.com:NicolasHug/vision…

de51d9f

… into remove_deprecated_0_14

NicolasHug marked this pull request as ready for review July 28, 2022 09:48

NicolasHug mentioned this pull request Sep 22, 2022

[WIP] Remove deprecated things that scheduled to be removed in v0.14 #6625

Closed

Merge branch 'main' into remove_deprecated_0_14

2f8d6f4

datumbox reviewed Sep 22, 2022

View reviewed changes

YosuaMichael added 2 commits September 22, 2022 12:24

Add back _transforms_video and change warning message

137d1b9

Change back the warning message, and will change the warning message …

3c4ba46

…on separate PR

YosuaMichael mentioned this pull request Sep 22, 2022

Remove cpp model in v0.14 due to deprecation #6632

Merged

NicolasHug mentioned this pull request Sep 22, 2022

Add missing handle_legacy_interface() calls #6565

Merged

Merge branch 'main' into remove_deprecated_0_14

12a8b24

NicolasHug commented Sep 26, 2022

View reviewed changes

datumbox approved these changes Sep 26, 2022

View reviewed changes

torchvision/models/mobilenetv2.py Show resolved Hide resolved

torchvision/models/mobilenetv3.py Show resolved Hide resolved

YosuaMichael mentioned this pull request Sep 27, 2022

Update the expected removal date for several deprecated API for release v0.14 #6654

Merged

YosuaMichael added 2 commits September 28, 2022 12:00

Merge branch 'main' into remove_deprecated_0_14

4963a07

Merge branch 'main' into remove_deprecated_0_14

6a09988

YosuaMichael merged commit 7b8a6db into pytorch:main Sep 28, 2022

YosuaMichael added deprecation module: ops module: datasets module: models module: transforms module: utils labels Sep 28, 2022

pmeier mentioned this pull request May 21, 2023

make_grid doesn't use kwargs #7612

Closed

		"The vision::models namespace is not actively maintained, use at "
		"your own discretion. We recommend using Torch Script instead: "



		warnings.warn(
		"The 'torchvision.transforms._functional_video' module is deprecated since 0.12 and will be removed in 0.14. "

Remove deprecated APIs for 0.14 #6258

Remove deprecated APIs for 0.14 #6258

Uh oh!

Conversation

NicolasHug commented Jul 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented Jul 28, 2022

Uh oh!

jdsgomes commented Aug 3, 2022

Uh oh!

NicolasHug commented Aug 3, 2022

Uh oh!

datumbox commented Aug 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdsgomes Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YosuaMichael commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

datumbox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NicolasHug commented Sep 26, 2022

Uh oh!

YosuaMichael commented Sep 27, 2022

Uh oh!

YosuaMichael commented Sep 28, 2022

Uh oh!

github-actions bot commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

NicolasHug Sep 22, 2022 •

edited

Loading

jdsgomes Sep 22, 2022 •

edited

Loading

YosuaMichael commented Sep 26, 2022 •

edited

Loading