Skip to content
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.

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

Improve Python module testing and cleanup memory leaks #26353

Merged
merged 14 commits into from
Jan 8, 2025

Conversation

jabraham17
Copy link
Member

@jabraham17 jabraham17 commented Dec 4, 2024

Make more of the Python package module tests run more frequently. While doing this, I also cleaned up as many of the Python memory leaks in the Python module as I could find.

Some of the tests rely on external Python dependencies like torch and numba. Previously, these tests were always skipped and a "compileOnly" version was tested. This PR adds a step to the skipif to install the dependencies locally if they are missing. This allows the tests to actually be executed in more cases

This PR also adds support for checking for unfree'ed references, controlled by the --pyMemLeaks flag. This relies on an external python dependency, so this uses similar external Python dependencies testing support as above

Testing

  • start_test test/library/packages/Python
  • start_test -memleaks test/library/packages/Python

Future Work:

  • remove global toFree list

[Reviewed by @DanilaFe]

@jabraham17 jabraham17 changed the title Improve Python module testing and add memLeaks testing Improve Python module testing and cleanup memory leaks Dec 4, 2024
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
@jabraham17 jabraham17 force-pushed the python-add-leak-testing branch from bf60294 to 5c0e566 Compare December 16, 2024 16:14
@jabraham17 jabraham17 marked this pull request as ready for review December 16, 2024 16:15
@jabraham17 jabraham17 requested a review from DanilaFe January 6, 2025 17:39
Copy link
Contributor

@DanilaFe DanilaFe left a comment

Choose a reason for hiding this comment

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

Looks good generally, per our conversation the CLEARs might need a revisit in future PRs.

modules/packages/Python.chpl Outdated Show resolved Hide resolved
modules/packages/Python.chpl Outdated Show resolved Hide resolved
util/test/sub_test.py Outdated Show resolved Hide resolved
util/test/sub_test.py Outdated Show resolved Hide resolved
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
Signed-off-by: Jade Abraham <[email protected]>
@jabraham17 jabraham17 merged commit 7faa9f4 into chapel-lang:main Jan 8, 2025
8 checks passed
@jabraham17 jabraham17 deleted the python-add-leak-testing branch January 8, 2025 00:23
@bradcray
Copy link
Member

bradcray commented Jan 8, 2025

@jabraham17 : Would I/we want to (and be able to) leverage the work here in the Chapel 2.3 blog release announcement to test more of the GPU code? IIRC, we disabled the testing of the first example due to its use of PyTorch?

@jabraham17
Copy link
Member Author

Very much so, all that should be required is to copy over a few test files like the pytorch .skipif, skipIfAndInstallPackage.sh, and the improved EXECENV. Nothing else should need to change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants