Skip to content

[dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]#119107

Closed
williamwen42 wants to merge 9 commits intogh/williamwen42/20/basefrom
gh/williamwen42/20/head
Closed

[dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]#119107
williamwen42 wants to merge 9 commits intogh/williamwen42/20/basefrom
gh/williamwen42/20/head

Conversation

@williamwen42
Copy link
Member

@williamwen42 williamwen42 commented Feb 3, 2024

Stack from ghstack (oldest at bottom):

Attempt #2 for #117875 to fix #112090.

Summary of changes:

  • Changed CacheEntry linked list into a doubly-linked list structure to support deletion. (done by C++ refactor)
  • Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
  • Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted. (done by C++ refactor)
  • CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
  • code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
  • Added tests that check for memory leaks and cache deletion operations.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng

@pytorch-bot
Copy link

pytorch-bot bot commented Feb 3, 2024

🔗 Helpful Links

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

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit 90a5192 with merge base c814d8e (image):

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

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

…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
williamwen42 added a commit that referenced this pull request Feb 3, 2024
… [attempt 2]

ghstack-source-id: 8adefac
Pull Request resolved: #119107
Copy link
Contributor

@jansel jansel left a comment

Choose a reason for hiding this comment

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

Tests failing?

…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
williamwen42 added a commit that referenced this pull request Feb 6, 2024
… [attempt 2]

ghstack-source-id: eb73423
Pull Request resolved: #119107
@williamwen42 williamwen42 added topic: not user facing topic category bug topic: bug fixes topic category and removed bug topic: bug fixes topic category topic: not user facing topic category labels Feb 6, 2024
…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
williamwen42 added a commit that referenced this pull request Feb 6, 2024
… [attempt 2]

ghstack-source-id: 998a425
Pull Request resolved: #119107
@williamwen42 williamwen42 requested a review from jansel February 6, 2024 18:06
@williamwen42
Copy link
Member Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Feb 6, 2024
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@williamwen42
Copy link
Member Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch cherry-pick -x 7dc9300dcced5f438a411caeb08dd3a8548855cf returned non-zero exit code 1

Auto-merging test/dynamo/test_misc.py
CONFLICT (content): Merge conflict in test/dynamo/test_misc.py
Auto-merging torch/_C/_dynamo/eval_frame.pyi
CONFLICT (content): Merge conflict in torch/_C/_dynamo/eval_frame.pyi
Auto-merging torch/_dynamo/__init__.py
CONFLICT (content): Merge conflict in torch/_dynamo/__init__.py
Auto-merging torch/_dynamo/eval_frame.py
Auto-merging torch/_dynamo/symbolic_convert.py
CONFLICT (modify/delete): torch/csrc/dynamo/cache_entry.cpp deleted in HEAD and modified in 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]).  Version 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]) of torch/csrc/dynamo/cache_entry.cpp left in tree.
CONFLICT (modify/delete): torch/csrc/dynamo/cache_entry.h deleted in HEAD and modified in 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]).  Version 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]) of torch/csrc/dynamo/cache_entry.h left in tree.
CONFLICT (modify/delete): torch/csrc/dynamo/extra_state.cpp deleted in HEAD and modified in 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]).  Version 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]) of torch/csrc/dynamo/extra_state.cpp left in tree.
CONFLICT (modify/delete): torch/csrc/dynamo/extra_state.h deleted in HEAD and modified in 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]).  Version 7dc9300dcce ([dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]) of torch/csrc/dynamo/extra_state.h left in tree.
Auto-merging torch/csrc/dynamo/init.cpp
CONFLICT (content): Merge conflict in torch/csrc/dynamo/init.cpp
error: could not apply 7dc9300dcce... [dynamo] delete dynamo cache entry when guard function is invalidated [attempt 2]
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
Details for Dev Infra team Raised by workflow job

…invalidated [attempt 2]"


Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng

[ghstack-poisoned]
williamwen42 added a commit that referenced this pull request Feb 7, 2024
… [attempt 2]

ghstack-source-id: f1d2e48
Pull Request resolved: #119107
@williamwen42
Copy link
Member Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

pytorch-bot bot pushed a commit that referenced this pull request Feb 8, 2024
… [attempt 2] (#119107)

Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

Pull Request resolved: #119107
Approved by: https://github.com/jansel
clee2000 pushed a commit that referenced this pull request Feb 14, 2024
… [attempt 2] (#119107)

Attempt #2 for #117875 to fix #112090.

Summary of changes:
- ~Changed CacheEntry linked list into a doubly-linked list structure to support deletion.~ (done by C++ refactor)
- Added CacheEntry and ExtraState borrowed references to GuardFn so that GuardFn can tell ExtraState to delete CacheEntry when the GuardFn is invalidated.
- ~Added ExtraState raw reference to CacheEntry so that we can get ExtraState to correctly point to the first CacheEntry if it gets deleted.~ (done by C++ refactor)
- CacheEntry destructor needs to reset GuardFn refs to ExtraState/CacheEntry in order to prevent use-after-free.
- code_context values that are nn.GraphModules need to be weakrefs in order to prevent circular references.
- Added tests that check for memory leaks and cache deletion operations.

Pull Request resolved: #119107
Approved by: https://github.com/jansel
@github-actions github-actions bot deleted the gh/williamwen42/20/head branch March 9, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo topic: bug fixes topic category topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants