Repository navigation
TST: measure retained reducer allocations instead of peak RSS - #602
Conversation
|
Hi, thank you for working on this.
This could be a personal note but it feels off here. Could you please disclose any LLM use in this PR ? Thank you. |
|
Thanks for the review. Yes, this change was drafted with LLM assistance (Instinct assisted), and it was reviewed and tested before submitting. I removed the typecheck sentence from the description. |
| bn.nanmax(arr, axis=axis) | ||
| bn.nanvar(arr, axis=axis) | ||
|
|
||
| hammer(50) |
There was a problem hiding this comment.
the purpose (warmup) is clear in the context of reviewing the PR, but I think a comment might help future readers too
There was a problem hiding this comment.
Good point, added a short comment above the warmup explaining it.
neutrinoceros
left a comment
neutrinoceros
left a comment
There was a problem hiding this comment.
looks good to me now. Though I'll wait 'til https://docs.python.org/3/library/tracemalloc.html is back online in order to run one last sanity check before I merge. Thanks again !
|
|
||
| monkeypatch.setattr(bn, "nansum", leaking_nansum) | ||
| with pytest.raises(AssertionError): | ||
| test_memory_leak() |
There was a problem hiding this comment.
The idea I can get behind, but this should really use pytest.mark.xfail(..., strict=True). It would make even more sense to parameterize the original test on the mocked function (one case being effectively "no-mock"), using pytest.param(..., marks=pytest.mark.xfail(..., strict=True) as the second case, replacing this one.
neutrinoceros
left a comment
neutrinoceros
left a comment
There was a problem hiding this comment.
I actually would like another change in before merging. Though I could handle it myself if you'd like.
|
Thanks, done in 0bfa920: |
| if leaky: | ||
| monkeypatch.setattr(bn, "nansum", _leaking_nansum_factory()) |
There was a problem hiding this comment.
what I had in mind was to use bn.nansum and _leaking_nansum_function directly in the parametrization and to mock unconditionally (the first case would obviously correspond to no mocks)
|
Got it, thanks. Reworked in 78d4cfc: |
| import gc | ||
| import tracemalloc |
There was a problem hiding this comment.
resource couldn't be imported portably at the top level but these could and should
|
Done in 2932f2a: |
|
pre-commit.ci autofix |
Fixes #597.
The reducer memory test compares process-wide peak RSS before and after its loop. Unrelated growth can fail that check, while a retained allocation can be missed if it stays below the previous peak.
Measure retained Python and NumPy allocations with tracemalloc after warming up the reducers and collecting garbage. Keep the same reducer calls and axes. Use the noise allowance already established by the error-path leak test and always stop tracing. Remove the Windows exclusion because resource is no longer used.
One test checks that the probe detects retained NumPy arrays.
Validation: 210 repository tests passed on Python 3.12.12 and NumPy 2.5.3 on macOS. The final 10-test memory suite, Ruff lint and formatting, and all configured pre-commit hooks passed.
This does not detect arbitrary untracked C allocations. Windows and free-threaded builds were not tested locally.