Fix memory leak in _LRScheduler.step() #85602

KinglittleQ · 2022-09-25T07:05:31Z

Fixes #85410

This diff removed the cyclic references in _LRScheduler.step().

pytorch-bot · 2022-09-25T07:05:33Z

🔗 Helpful Links

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

📄 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, 1 Pending

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

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

albanD · 2022-09-26T17:10:59Z

The change sounds ok but the CI failures are real right?

KinglittleQ · 2022-09-28T13:06:16Z

Thanks for your comments. I will fix the CI.

KinglittleQ · 2022-09-28T13:06:27Z

Thanks for your comments. I will fix the CI.

facebook-github-bot · 2022-10-04T00:13:20Z

/easycla

As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details.

This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign.

linux-foundation-easycla · 2022-10-04T00:13:24Z

The committers listed above are authorized under a signed CLA.

✅ login: KinglittleQ / name: Chengqi Deng (32fa903, 0c8eced)

KinglittleQ · 2022-10-05T06:19:57Z

@albanD There are some updates to try to fix the tests. Could you please take another look?

albanD · 2022-10-05T16:02:31Z

Thanks!
(GitHub is in a bad state right now, so CI might take a bit of time to trigger properly).

albanD · 2022-10-05T16:03:28Z

test/test_optim.py

+        import gc
+        gc.collect()
+        run()
+        garbage = gc.collect()


You don't want the collect here I think?
THe whole point here is not to create ref cycles right?

Yes, if there are any ref cycles created in run(), the second gc.collect() would return non-zero.

My bad, I was still under the idea of using weakref and didn't read the update carefully. Sounds good!

KinglittleQ · 2022-10-06T11:47:42Z

@albanD The CI needs to be triggered by a maintainer since I am a first-time contributor. Could you take a look?

albanD

Looks good thanks

albanD · 2022-10-06T14:07:54Z

test/test_optim.py

+        import gc
+        gc.collect()
+        run()
+        garbage = gc.collect()


My bad, I was still under the idea of using weakref and didn't read the update carefully. Sounds good!

albanD · 2022-10-06T14:08:39Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-06T14:10:53Z

@pytorchbot successfully started a merge job. Check the current status here.
The merge job was triggered with the green (-g) flag. This means that your change will be merged once all checks on your PR have passed (ETA: 0-4 Hours). If this is not the intended behavior, feel free to use some of the other merge options in the wiki.
Please reach out to the PyTorch DevX Team with feedback or questions!

albanD · 2022-10-06T21:59:51Z

@pytorchbot -h

pytorch-bot · 2022-10-06T21:59:53Z

PyTorchBot Help

usage: @pytorchbot [-h] {merge,revert,rebase,label} ...

In order to invoke the bot on your PR, include a line that starts with
@pytorchbot anywhere in a comment. That line will form the command; no
multi-line commands are allowed. 

Example:
    Some extra context, blah blah, wow this PR looks awesome

    @pytorchbot merge

optional arguments:
  -h, --help            Show this help message and exit.

command:
  {merge,revert,rebase,label}
    merge               Merge a PR
    revert              Revert a PR
    rebase              Rebase a PR
    label               Add label to a PR

Merge

usage: @pytorchbot merge [-g | -f MESSAGE | -l] [-r [{viable/strict,master}]]

Merge an accepted PR, subject to the rules in .github/merge_rules.json.
By default, this will wait for all required checks (lint, pull) to succeed before merging.

optional arguments:
  -g, --green           Merge when all status checks running on the PR pass. To add status checks, use labels like `ciflow/trunk`.
  -f MESSAGE, --force MESSAGE
                        Merge without checking anything. This requires a reason for auditting purpose, for example:
                        @pytorchbot merge -f 'Minor update to fix lint. Expecting all PR tests to pass'
  -l, --land-checks     [Deprecated - your PR instead now gets the `ciflow/trunk` label on approval] Merge with land time checks. This will create a new branch with your changes rebased on viable/strict and run a majority of trunk tests _before_ landing to increase trunk reliability and decrease risk of revert. The tests added are: pull, Lint and trunk. Note that periodic is excluded.
  -r [{viable/strict,master}], --rebase [{viable/strict,master}]
                        Rebase the PR to re run checks before merging.  Accepts viable/strict or master as branch options and will default to viable/strict if not specified.

Revert

usage: @pytorchbot revert -m MESSAGE -c
                          {nosignal,ignoredsignal,landrace,weird,ghfirst}

Revert a merged PR. This requires that you are a Meta employee.

Example:
  @pytorchbot revert -m="This is breaking tests on trunk. hud.pytorch.org/" -c=nosignal

optional arguments:
  -m MESSAGE, --message MESSAGE
                        The reason you are reverting, will be put in the commit message. Must be longer than 3 words.
  -c {nosignal,ignoredsignal,landrace,weird,ghfirst}, --classification {nosignal,ignoredsignal,landrace,weird,ghfirst}
                        A machine-friendly classification of the revert reason.

Rebase

usage: @pytorchbot rebase [-s | -b BRANCH]

Rebase a PR. Rebasing defaults to the stable viable/strict branch of pytorch.
You, along with any member of the pytorch organization, can rebase your PR.

optional arguments:
  -s, --stable          [DEPRECATED] Rebase onto viable/strict
  -b BRANCH, --branch BRANCH
                        Branch you would like to rebase to

Label

usage: @pytorchbot label labels [labels ...]

Adds label to a PR

positional arguments:
  labels  Labels to add to given Pull Request

albanD · 2022-10-06T22:00:33Z

@pytorchbot revert -m "newly added test is flaky" -c nosignal

albanD · 2022-10-06T22:00:51Z

For more information on the flaky test, see #86413

pytorchmergebot · 2022-10-06T22:01:58Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2022-10-06T22:02:06Z

@KinglittleQ your PR has been successfully reverted.

This reverts commit eb32330. Reverted #85602 on behalf of https://github.com/albanD due to newly added test is flaky

albanD · 2022-10-06T22:02:36Z

@KinglittleQ my best guess of the flakyness here is that if any other object happens to be ready for collection when you run gc.collect(), it will fail the test even though we didn't create a cycle in the function.

Could you update the test to use weakref again so that we make sure that: while the gc is disabled, the weakef remains alive after the function AND after running the gc manually the weakref is gone.

…Q/pytorch into fix-memoryleak-in-lrscheduler

KinglittleQ · 2022-10-07T10:01:59Z

@albanD The test has been updated now.

KinglittleQ · 2022-10-07T10:05:53Z

test/test_optim.py

+        gc.disable()
+        ref = run()
+        assert ref() is None
+        gc.enable()  # restore


The scheduler object should be freed after run() if there are no ref cycles.

This test ensures that the weakref will be unavailable even if the gc is disabled. (gc would collect objects with ref cycles)

albanD

Perfect! Thanks!

albanD · 2022-10-07T15:54:31Z

@pytorchbot merge -g

pytorchmergebot · 2022-10-07T15:55:51Z

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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

Summary: Fixes #85410 This diff removed the cyclic references in `_LRScheduler.step()`. Pull Request resolved: #85602 Approved by: https://github.com/albanD Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/b3fdb02fb25508d9c61d70b594f8a7fac3b2a365 Reviewed By: seemethere Differential Revision: D40197030 Pulled By: seemethere fbshipit-source-id: ed4be542b6f19bc1030d0ae74b40994a055cff29

Fix memory leak in LRScheduler

32fa903

KinglittleQ requested a review from albanD as a code owner September 25, 2022 07:05

facebook-github-bot added the cla signed label Sep 25, 2022

pytorchbot added the open source label Sep 25, 2022

zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 26, 2022

Fix test

0c8eced

albanD reviewed Oct 5, 2022

View reviewed changes

Merge branch 'pytorch:master' into fix-memoryleak-in-lrscheduler

084a397

albanD approved these changes Oct 6, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 6, 2022

albanD added release notes: python_frontend release notes category topic: improvements topic category labels Oct 6, 2022

pytorchmergebot added the Merged label Oct 6, 2022

pytorchmergebot closed this in eb32330 Oct 6, 2022

pytorchmergebot added the Reverted label Oct 6, 2022

pytorchmergebot added a commit that referenced this pull request Oct 6, 2022

Revert "Fix memory leak in _LRScheduler.step() (#85602)"

233d6f1

This reverts commit eb32330. Reverted #85602 on behalf of https://github.com/albanD due to newly added test is flaky

albanD reopened this Oct 6, 2022

KinglittleQ added 2 commits October 7, 2022 15:24

Try to fix gc test

1b1c2d1

Merge branch 'fix-memoryleak-in-lrscheduler' of github.com:Kinglittle…

266ac1b

…Q/pytorch into fix-memoryleak-in-lrscheduler

KinglittleQ commented Oct 7, 2022

View reviewed changes

albanD approved these changes Oct 7, 2022

View reviewed changes

pytorchmergebot closed this in b3fdb02 Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in _LRScheduler.step() #85602

Fix memory leak in _LRScheduler.step() #85602

KinglittleQ commented Sep 25, 2022

pytorch-bot bot commented Sep 25, 2022 •

edited

albanD commented Sep 26, 2022

KinglittleQ commented Sep 28, 2022

KinglittleQ commented Sep 28, 2022

facebook-github-bot commented Oct 4, 2022

linux-foundation-easycla bot commented Oct 4, 2022 •

edited

KinglittleQ commented Oct 5, 2022

albanD commented Oct 5, 2022

albanD Oct 5, 2022

KinglittleQ Oct 6, 2022

albanD Oct 6, 2022

KinglittleQ commented Oct 6, 2022

albanD left a comment

albanD Oct 6, 2022

albanD commented Oct 6, 2022

pytorchmergebot commented Oct 6, 2022

albanD commented Oct 6, 2022

pytorch-bot bot commented Oct 6, 2022

albanD commented Oct 6, 2022

albanD commented Oct 6, 2022

pytorchmergebot commented Oct 6, 2022

pytorchmergebot commented Oct 6, 2022

albanD commented Oct 6, 2022

KinglittleQ commented Oct 7, 2022

KinglittleQ Oct 7, 2022

albanD left a comment

albanD commented Oct 7, 2022

pytorchmergebot commented Oct 7, 2022

Fix memory leak in _LRScheduler.step() #85602

Fix memory leak in _LRScheduler.step() #85602

Conversation

KinglittleQ commented Sep 25, 2022

pytorch-bot bot commented Sep 25, 2022 • edited

🔗 Helpful Links

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

✅ No Failures, 1 Pending

albanD commented Sep 26, 2022

KinglittleQ commented Sep 28, 2022

KinglittleQ commented Sep 28, 2022

facebook-github-bot commented Oct 4, 2022

linux-foundation-easycla bot commented Oct 4, 2022 • edited

KinglittleQ commented Oct 5, 2022

albanD commented Oct 5, 2022

albanD Oct 5, 2022

Choose a reason for hiding this comment

KinglittleQ Oct 6, 2022

Choose a reason for hiding this comment

albanD Oct 6, 2022

Choose a reason for hiding this comment

KinglittleQ commented Oct 6, 2022

albanD left a comment

Choose a reason for hiding this comment

albanD Oct 6, 2022

Choose a reason for hiding this comment

albanD commented Oct 6, 2022

pytorchmergebot commented Oct 6, 2022

albanD commented Oct 6, 2022

pytorch-bot bot commented Oct 6, 2022

PyTorchBot Help

Merge

Revert

Rebase

Label

albanD commented Oct 6, 2022

albanD commented Oct 6, 2022

pytorchmergebot commented Oct 6, 2022

pytorchmergebot commented Oct 6, 2022

albanD commented Oct 6, 2022

KinglittleQ commented Oct 7, 2022

KinglittleQ Oct 7, 2022

Choose a reason for hiding this comment

albanD left a comment

Choose a reason for hiding this comment

albanD commented Oct 7, 2022

pytorchmergebot commented Oct 7, 2022

Merge started

pytorch-bot bot commented Sep 25, 2022 •

edited

linux-foundation-easycla bot commented Oct 4, 2022 •

edited