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

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

Merged

Conversation

YosuaMichael
Copy link
Contributor

@YosuaMichael YosuaMichael commented Sep 27, 2022

Related to discussion: #6258 (comment)

  • Delay the removal for transforms/_functional_video.py and transforms/_transforms_video.py since we dont have alternatives, and we remove the expected date until we have alternatives and know we can safely remove these files.

Let me know if you have any opinion on this!

@NicolasHug
Copy link
Member

NicolasHug commented Sep 27, 2022

Thanks Yosua.

Following our internal discussions, the changes relating to the transforms are not controversial.

However, I have reservations about removing the expected removal date of pretrained=True, which I started formulating in the original issue #6564 (comment). From my understanding there are a few different scenarios for downstream libraries using torchvision:

  • a) They're fine pinning to the latest version. Great, we don't need to do anyting.

  • b) They want to use torchvision>=0.13 eventually, but need a bit of time to migrate from pretrained=True to weights=..., and they want to do that uniformly. They (understandably) need a bit more time than the 2 version cycles we give them. To address these we can treat this as an exception, and commit to removal in 0.16 (or 0.17 or whatever) instead of the current 0.15.

  • c) They want to support torchvision>=SOME_OLD_VERSION where SOME_OLD_VERSION < 0.13, as e.g. required if they want to support the LTS torch core version. These ones have a problem: they can't support 0.13+ onward unless they write custom code to account for the fact that pretrained=True will be removed in 0.15.

I personally think addressing b) by upping the targetted removal to to e.g. 0.16 (or 0.17) is sensible, as long as we treat this as exceptional. But this PR tries to address c) by not committing to a removal date - this is where I'm becoming less comfortable with it.

First, a sad observation:

they can't support 0.13+ onward unless they write custom code to account for the fact that pretrained=True will be removed in 0.15.

This is what all libraries do. That's the sad reality of being a downstream libs: you have to write custom code to navigate deprecations and handle different versions of your dependencies. We have to deal with that ourselves in torchvision for our PIL dependency!

Next, some concerns: by not committing to a removal date, we are i) being inconsistent with our deprecation policy and the rest of our deprecated APIs, and ii) not encouraging downstream libraries to migrate: why would they migrate if they don't have to? That can slow down adoption too.

On top of that, not committing to a removal date essentially means that we'll only be able to remove these APIs after all downstream libs have migrated. Since we can't keep track of all of them, that essentially means, IIUC, that we can only remove these once a new LTS pops up?

Perhaps I'm missing or misunderstanding some things, but this change seems to open the door to various unexpected and undesirable situations (which go beyond torchvision BTW, since our deprecation policy comes straight from that of core). If possible, I'd like to discuss this more with the team in details and align on a path forward, where everyone is on the same page regarding the pros and cons of each alternatives.

@YosuaMichael
Copy link
Contributor Author

YosuaMichael commented Sep 27, 2022

@NicolasHug thanks for writing up your opinion, and I think it make sense and I agree that having "when" we will remove it can give urgency to downstream users (otherwise they will prefer not to change) and if we should have it if we really want the user to migrate to the new API.

That being said, I think for pretrained=True it is widely used and I think it need more than a warning on the code if we want to remove it. We should have a bigger announcement (article or blog) which tell people we will remove pretrained=True from torchvision in version x.x.x. (In general, I think deprecation and removal for a widely used API should have a bigger announcement)

And for us to decide when is the good version to remove, it also need effort to estimate how much commitment to follow up after that (to give support to the internal users and oss) to make sure we dont break too much things. And I think until release v0.14 we dont have enough time to decide when is the good version to remove.

In my opinion the other option that we can do:

  1. Don't change the removal version (still v0.15) but we may change them later when we release v0.15 if we need more time
  2. We change the wording to may and we may change it later on v0.15 release:
"The 'torchvision.transforms._functional_video' module is deprecated since 0.12 and may be removed in v0.15 "

@NicolasHug
Copy link
Member

That being said, I think for pretrained=True it is widely used and I think it need more than a warning on the code if we want to remove it. We should have a bigger announcement (article or blog)

You're right for big changes or major features, it's worth raising awareness outside of the simple deprecation warnings. Vasilis has done a great job at that already, and the deprecation was announced in the following blog post: https://pytorch.org/blog/introducing-torchvision-new-multi-weight-support-api/#deprecations. The new API itself was announced in other posts / tweets / RFCs / release highlights so there's quite a bit of documentation about it already (and the fact that it replaces the old one)

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM, the proposed change aligns with our internal discussions. This was not an easy choice but we should unblock the PR to prepare the upcoming release.

@YosuaMichael YosuaMichael merged commit 2907c49 into pytorch:main Sep 29, 2022
@github-actions
Copy link

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

facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2022
…for release v0.14 (#6654)

Summary:
* Update the expected removal date for several deprecated API

* Revert the change in models/_utils.py

* Remove removal date on pretrained=True

* Update another message related to pretrained=True

* Also update the warning in kwonly_to_pos_or_kw decorator

* Update remaining message in _utils.py

Reviewed By: datumbox

Differential Revision: D40138732

fbshipit-source-id: 02d168495b8964b85519266ba1d9f773ed634351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants