Improve fake tensor leakage detection in export by not relying on gc too much#163516
Improve fake tensor leakage detection in export by not relying on gc too much#163516tugsbayasgalan wants to merge 2 commits intogh/tugsbayasgalan/38/basefrom
Conversation
…too much [ghstack-poisoned]
tugsbayasgalan
requested review from
angelayi,
avikchaudhuri,
ydwu4 and
zhxchen17
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163516
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit e727818 with merge base 0b59492 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@tugsbayasgalan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
cc @eellison for fake tensor constructor modifications |
eellison
left a comment
eellison
left a comment
There was a problem hiding this comment.
why do we track constructors specifically ? We already track every fake tensor in the fake tensor converter.
|
Ah i didn't think about fake tensor converter lol, let me check |
…ying on gc too much" Previously we relied on gc to get the snapshot of fake tensors before and after export to get list of fake tensors that are created during export. This caused some flakiness in our test suite (#162232). it seems super hard to make gc deterministic, so we just instrument fake tensor creation which seems lot better. In addition, it is also quite faster than previous approach becuase we are no longer manually triggering garbage collector. cc ezyang EikanWang jgong5 wenzhe-nrv Differential Revision: [D82966648](https://our.internmc.facebook.com/intern/diff/D82966648) [ghstack-poisoned]
|
@tugsbayasgalan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
As @eellison discovered, we construct fake tensors in fake_impls as well, so fake tensor converter might not be the most reliable place for this instrumentation. |
|
@pytorchbot merge -f "Landed internally" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
|
The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command |
|
Can't merge closed PR #163516 |
…too much (pytorch#163516) Previously we relied on gc to get the snapshot of fake tensors before and after export to get list of fake tensors that are created during export. This caused some flakiness in our test suite (pytorch#162232). it seems super hard to make gc deterministic, so we just instrument fake tensor creation which seems lot better. In addition, it is also quite faster than previous approach becuase we are no longer manually triggering garbage collector. Differential Revision: [D82966648](https://our.internmc.facebook.com/intern/diff/D82966648) Pull Request resolved: pytorch#163516 Approved by: https://github.com/ezyang
5 participants
Stack from ghstack (oldest at bottom):
Previously we relied on gc to get the snapshot of fake tensors before and after export to get list of fake tensors that are created during export. This caused some flakiness in our test suite (#162232). it seems super hard to make gc deterministic, so we just instrument fake tensor creation which seems lot better. In addition, it is also quite faster than previous approach becuase we are no longer manually triggering garbage collector.
cc @ezyang @EikanWang @jgong5 @wenzhe-nrv
Differential Revision: D82966648