Skip to content

Enable Leak Sanitizer #154584

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Enable Leak Sanitizer #154584

wants to merge 3 commits into from

Conversation

cyyever
Copy link
Collaborator

@cyyever cyyever commented May 29, 2025 •
edited
Loading

It enables Leak Sanitizer and also provides a suppression file.
Before LSAN, the longest ASAN job is around 52min; after LSAN the average ASAN job time is more than 2h 30min.

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

@cyyever cyyever requested review from a team, albanD and soulitzer as code owners May 29, 2025 02:53
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 29, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 12a702e with merge base c699668 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@cyyever
Copy link
Collaborator Author

cyyever commented May 29, 2025

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label May 29, 2025
@cyyever cyyever marked this pull request as draft May 29, 2025 11:15
@cyyever
Copy link
Collaborator Author

cyyever commented Jun 17, 2025

@pytorchbot rebase -b main

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/main. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased lsan2 onto refs/remotes/origin/main, please pull locally before adding more changes (for example, via git checkout lsan2 && git pull --rebase)

@cyyever
Copy link
Collaborator Author

cyyever commented Jun 18, 2025

@pytorchbot rebase -b main

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/main. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased lsan2 onto refs/remotes/origin/main, please pull locally before adding more changes (for example, via git checkout lsan2 && git pull --rebase)

albanD
albanD reviewed Jun 20, 2025
View reviewed changes
torch/csrc/autograd/VariableTypeManual.cpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was that a leak?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previous run reported possible leaks here

torch/csrc/autograd/variable.h Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, are these leaks?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previous run reported possible leaks here, should be false positive. Nevertheless, the move avoids one copy operation.

@cyyever cyyever marked this pull request as ready for review June 21, 2025 11:10
@cyyever cyyever force-pushed the lsan2 branch 4 times, most recently from 146093b to 7448128 Compare June 22, 2025 00:33
@cyyever cyyever requested a review from ezyang June 22, 2025 10:08
ezyang
ezyang previously approved these changes Jun 23, 2025
View reviewed changes
@ezyang
Copy link
Contributor

ezyang commented Jun 23, 2025

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jun 23, 2025
@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

@cyyever
Copy link
Collaborator Author

cyyever commented Jun 23, 2025

@pytorchbot revert -m "Need to suppress more output" -c nosignal

@pytorchmergebot
Copy link
Collaborator

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

pytorchmergebot added a commit that referenced this pull request Jun 23, 2025
@pytorchmergebot
Revert "Enable Leak Sanitizer (#154584)"
f5e1b24 
This reverts commit c79c7bb.

Reverted #154584 on behalf of https://github.com/cyyever due to Need to suppress more output ([comment](#154584 (comment)))
@pytorchmergebot
Copy link
Collaborator

@cyyever your PR has been successfully reverted.

@pytorchmergebot pytorchmergebot added Reverted ci-no-td Do not run TD on this PR labels Jun 23, 2025
@cyyever cyyever force-pushed the lsan2 branch 2 times, most recently from 6cd775a to 85fa74a Compare June 23, 2025 11:05
@malfet malfet dismissed ezyang’s stale review June 23, 2025 14:31

Can you please post how much overhead does it add to the tests?

malfet
malfet requested changes Jun 23, 2025
View reviewed changes
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Please add an additional overhead this leak checker adds to the CI. Also, if we have this perhaps we should get rid of leak check jobs that we are running periodically?

@cyyever
Copy link
Collaborator Author

cyyever commented Jun 25, 2025

Please add an additional overhead this leak checker adds to the CI. Also, if we have this perhaps we should get rid of leak check jobs that we are running periodically?

2 more hours to run CI.

@cyyever cyyever requested a review from malfet June 25, 2025 14:28
@ezyang
Copy link
Contributor

ezyang commented Jun 26, 2025

2hr is pretty stiff, unfortunately. Not sure what I would do here though, I don't know what the current latency breakdown for CI is right now.

@cyyever
Copy link
Collaborator Author

cyyever commented Jun 26, 2025

@ezyang LSAN should be moved to periodic jobs.

@soulitzer soulitzer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albanD albanD albanD left review comments

@ezyang ezyang ezyang left review comments

@soulitzer soulitzer Awaiting requested review from soulitzer soulitzer is a code owner

@malfet malfet Awaiting requested review from malfet

Assignees
No one assigned
Labels
ci-no-td Do not run TD on this PR ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo open source Reverted topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cyyever @pytorchmergebot @ezyang @malfet @albanD @soulitzer @pytorchbot