Publicly expose _LRScheduler to LRScheduler by janeyx99 · Pull Request #88503 · pytorch/pytorch

janeyx99 · 2022-11-04T16:50:46Z

Fixes #61232

pytorch-bot · 2022-11-04T16:50:49Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/88503

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 1ea11ec:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

soulitzer

Cool!

pytorch-bot · 2022-11-04T20:10:59Z

This PR has been accepted with the accept2ship label. Attempting to merge now.

@pytorchbot merge -l

pytorchmergebot · 2022-11-04T20:12:39Z

Merge started

The -l land checks flag is deprecated and no longer needed. Instead we now automatically add the ciflow\trunk label to your PR once it's approved

Your change will be merged once all checks on your PR pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

torch/optim/lr_scheduler.py

vadimkantorov · 2022-11-04T20:35:10Z

Related: #23957, #68332, one avenue is a redesign of LRScheduler and introduction of general scheduler interface and functional forms...

pytorchmergebot · 2022-11-05T02:01:17Z

Merge failed

Reason: 1 additional jobs have failed, first few of them are: trunk

Details for Dev Infra team

Raised by workflow job

janeyx99 · 2022-11-07T16:14:58Z

@pytorchbot merge

pytorchmergebot · 2022-11-07T16:16:41Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

janeyx99 · 2022-11-08T19:27:31Z

@vadimkantorov Thanks for bringing to attention the relevant LRScheduler issues, unfortunately, we don't have anyone focused + working on them today.

Summary: After pytorch/pytorch#88503 , these isinstance checks no longer pass. Example: https://github.com/pytorch/tnt/actions/runs/3434539858/jobs/5725945075 TODO: support older torch versions that don't have `LRScheduler` and need `_LRScheduler` Differential Revision: D41177335 fbshipit-source-id: 0fc2009fc5fd7289e95a37d7b417476d7c4f2b61

Following #88503, we should also update the pyi file Pull Request resolved: #88818 Approved by: https://github.com/soulitzer

Summary: Pull Request resolved: meta-pytorch#271 After pytorch/pytorch#88503 , these isinstance checks no longer pass. Example: https://github.com/pytorch/tnt/actions/runs/3434539858/jobs/5725945075 Reviewed By: janeyx99 Differential Revision: D41177335 fbshipit-source-id: 243b2fb4cdb06561588b56a9a9f08a7144c6ae9b

Summary: Pull Request resolved: meta-pytorch#271 After pytorch/pytorch#88503 , these isinstance checks no longer pass. Example: https://github.com/pytorch/tnt/actions/runs/3434539858/jobs/5725945075 Reviewed By: janeyx99 Differential Revision: D41177335 fbshipit-source-id: c09398e06bebc2e9362f369143d4d8dd7c21e6e6

Summary: Pull Request resolved: #271 After pytorch/pytorch#88503 , these isinstance checks no longer pass. Example: https://github.com/pytorch/tnt/actions/runs/3434539858/jobs/5725945075 Reviewed By: daniellepintz, janeyx99 Differential Revision: D41177335 fbshipit-source-id: a06cf2f1c34eb0e697df716d8ac6e393359e0c4c

Fixes pytorch#61232 Pull Request resolved: pytorch#88503 Approved by: https://github.com/soulitzer

Following pytorch#88503, we should also update the pyi file Pull Request resolved: pytorch#88818 Approved by: https://github.com/soulitzer

Summary: pytorch/pytorch#88503 introduces the public version `LRScheduler`, however `isinstance(self.scheduler, torch.optim.lr_scheduler._LRScheduler)` doesn't work anymore because of https://github.com/pytorch/pytorch/blob/1ea11ecb2eea99eb552603b7cf5fbfc59659832d/torch/optim/lr_scheduler.py#L166-L169. It's a bit tricky to make it BC compatible for torch version <= 1.13. V1 of this diff uses try catch block to import the `LRScheduler` and make it available in `detectron2.solver`, then the whole D2 (facebookresearch@11528ce) uses this version of `LRScheduler`. There're two drawbacks though: - it adds a little mental burden to figure out what's D2 (facebookresearch@11528ce083dc9ff83ee3a8f9086a1ef54d2a402f)'s `LRScheduler`, previously it's clear that the `LRScheduler`/`_LRScheduler` is from `torch`. - it has a name collision with `hooks.LRScheduler`, eg. in the `hooks.py` I have to do `LRScheduler as _LRScheduler`. But I couldn't found a better solution, maybe use try catch block in every file? Differential Revision: D42111273 fbshipit-source-id: 613cd35a1e0bdb9b8edd852042df047aba021d87

Summary: Pull Request resolved: #4709 pytorch/pytorch#88503 introduces the public version `LRScheduler`, however `isinstance(self.scheduler, torch.optim.lr_scheduler._LRScheduler)` doesn't work anymore because of https://github.com/pytorch/pytorch/blob/1ea11ecb2eea99eb552603b7cf5fbfc59659832d/torch/optim/lr_scheduler.py#L166-L169. It's a bit tricky to make it BC compatible for torch version <= 1.13. V1 of this diff uses try catch block to import the `LRScheduler` and make it available in `detectron2.solver`, then the whole D2 (11528ce) uses this version of `LRScheduler`. There're two drawbacks though: - it adds a little mental burden to figure out what's D2 (11528ce083dc9ff83ee3a8f9086a1ef54d2a402f)'s `LRScheduler`, previously it's clear that the `LRScheduler`/`_LRScheduler` is from `torch`. - it has a name collision with `hooks.LRScheduler`, eg. in the `hooks.py` I have to do `LRScheduler as _LRScheduler`. But I couldn't found a better solution, maybe use try catch block in every file? Reviewed By: sstsai-adl Differential Revision: D42111273 fbshipit-source-id: 0269127de1ba3ef90225c5dfe085bb209f6cf4d1

Summary: Introduce `torchtnt.utils.lr_scheduler.TLRScheduler` to manage compatibility of torch with expose of `LRScheduler` pytorch/pytorch#88503 It seems that issue persist still with `1.13.1` (see #285). Replace the get_version logic with try/except. `_LRscheduler`. Fixes #285 `https://github.com/pytorch/tnt/issues/285` Pull Request resolved: #286 Reviewed By: hudeven Differential Revision: D42220767 Pulled By: ananthsub fbshipit-source-id: 3b434ba4e0c8efa1a81ab5a88ae58765bb23020e

Publicly expose _LRScheduler to LRScheduler

1ea11ec

janeyx99 added the release notes: nn release notes category label Nov 4, 2022

janeyx99 marked this pull request as ready for review November 4, 2022 19:05

janeyx99 requested a review from albanD as a code owner November 4, 2022 19:05

janeyx99 requested review from mikaylagawarecki and soulitzer November 4, 2022 19:06

janeyx99 added topic: improvements topic category accept2ship labels Nov 4, 2022

soulitzer approved these changes Nov 4, 2022

View reviewed changes

pytorch-bot bot added ciflow/trunk Trigger trunk jobs on your pull request and removed accept2ship labels Nov 4, 2022

janeyx99 commented Nov 4, 2022

View reviewed changes

torch/optim/lr_scheduler.py Show resolved Hide resolved

vadimkantorov mentioned this pull request Nov 7, 2022

[naming] Promote _LRScheduler to LRScheduler #23957

Closed

pytorchmergebot added the Merged label Nov 7, 2022

pytorchmergebot closed this in 0a69c50 Nov 7, 2022

ananthsub mentioned this pull request Nov 10, 2022

Update LRScheduler instance checks meta-pytorch/tnt#271

Closed

janeyx99 mentioned this pull request Nov 10, 2022

Update lr_scheduler.pyi to match lr_scheduler.py #88818

Closed

janeyx99 mentioned this pull request Nov 14, 2022

Use LRScheduler instead of _LRScheduler pytorch/ignite#2773

Closed

wat3rBro mentioned this pull request Dec 16, 2022

update to use torch.optim.lr_scheduler.LRScheduler facebookresearch/detectron2#4709

Closed

dmtrs mentioned this pull request Dec 21, 2022

Create torchtnt.utils.lr_scheduler.TLRScheduler meta-pytorch/tnt#286

Closed

xkszltl mentioned this pull request Mar 27, 2024

Transformers 4.39.0 has breaking change on torch compatibility. huggingface/transformers#29895

Closed

github-actions bot deleted the lrscheduler_public branch May 6, 2024 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publicly expose _LRScheduler to LRScheduler#88503

Publicly expose _LRScheduler to LRScheduler#88503
janeyx99 wants to merge 1 commit intomasterfrom
lrscheduler_public

janeyx99 commented Nov 4, 2022

Uh oh!

pytorch-bot bot commented Nov 4, 2022 •

edited

Loading

Uh oh!

soulitzer left a comment

Uh oh!

pytorch-bot bot commented Nov 4, 2022

Uh oh!

pytorchmergebot commented Nov 4, 2022

Uh oh!

Uh oh!

vadimkantorov commented Nov 4, 2022 •

edited

Loading

Uh oh!

pytorchmergebot commented Nov 5, 2022

Uh oh!

janeyx99 commented Nov 7, 2022

Uh oh!

pytorchmergebot commented Nov 7, 2022

Uh oh!

janeyx99 commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

janeyx99 commented Nov 4, 2022

Uh oh!

pytorch-bot bot commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/88503

✅ No Failures

Uh oh!

soulitzer left a comment

Choose a reason for hiding this comment

Uh oh!

pytorch-bot bot commented Nov 4, 2022

Uh oh!

pytorchmergebot commented Nov 4, 2022

Merge started

Uh oh!

Uh oh!

vadimkantorov commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorchmergebot commented Nov 5, 2022

Merge failed

Uh oh!

janeyx99 commented Nov 7, 2022

Uh oh!

pytorchmergebot commented Nov 7, 2022

Merge started

Uh oh!

janeyx99 commented Nov 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pytorch-bot bot commented Nov 4, 2022 •

edited

Loading

vadimkantorov commented Nov 4, 2022 •

edited

Loading