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

Add tests for memory leaks #106

Closed
wants to merge 6 commits into from
Closed

Add tests for memory leaks #106

wants to merge 6 commits into from

Conversation

sethmlarson
Copy link
Owner

@sethmlarson sethmlarson commented Apr 23, 2023

Closes #104

@sethmlarson sethmlarson requested a review from davisagli as a code owner April 23, 2023 20:50
noxfile.py Outdated Show resolved Hide resolved


@pytest.mark.skipif(not memray_installed, reason="Memray isn't installed")
@pytest.mark.limit_memory("1MB")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is worth something, but for catching leaks what we care about is not the high water mark but how much RAM is still used after all tests are done...right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I agree, I was trying to figure out how we can assert that no allocations were leaked but wasn't able to immediately find anything, I see this PR which looks like what we need? Maybe I'll add a ping of @pablogsal in case there's something I'm missing from the docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sethmlarson Okay yeah, that looks like the right place for it. We can follow up and add it once that PR lands.

Choose a reason for hiding this comment

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

We are still iterating a bit over thar PR because is quite difficult to isolate from weird one-off allocations that the interpreter does and caches. Stay tuned. Meanwhile you can use the high watermark limit + a lot of iterations of the test as a proxy if you want

davisagli
davisagli previously approved these changes Apr 24, 2023
@sethmlarson
Copy link
Owner Author

(Sorry for review dismissal, wanted to see if this test is even capable of detecting leaked memory)

@sethmlarson
Copy link
Owner Author

Going to close this PR since it's stalled in case someone else wanted to try adding memray.

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

Successfully merging this pull request may close these issues.

Add memray to CI
3 participants