Repository navigation
docs: name peak memory as the MLX allocator peak - #132
Conversation
The benchmark table's "Peak active memory" was easy to read as process RSS. It is mlx.core.get_peak_memory(); rename the column, clarify Requirements, and match the bench.py labels. Fixes Edge0-AI#106
|
Thanks for tightening this and sorry for our late review. The column rename is the right fix: Three changes before this can land:
The |
- bench.py docstring: drop duplicated "MLX" - README Requirements: drop the ~2x process-footprint sentence; say the extra RSS is reclaimable and does not set the machine requirement - python/README.md: rename the "Peak active memory" column and Memory row - hygiene test: check both READMEs and assert "peak active" is gone
shuk777
left a comment
There was a problem hiding this comment.
Requesting changes on two edits. The column rename, the footnote, the bench.py print labels, and the new hygiene test are fine.
The module docstring now reads "MLX MLX", and the Requirements sentence about a 2× process footprint conflicts with the conclusion already recorded on #106.
| Runs the same measurement shape as the deployment's bench: prefill -> | ||
| sampled warmup steps -> timed sampled decode -> report tok/s and MLX | ||
| peak active memory. Works for both tiers; the tier is auto-detected | ||
| MLX allocator peak memory (`mlx.core.get_peak_memory`). Works for both tiers; the tier is auto-detected |
There was a problem hiding this comment.
The line above already ends with "and MLX", so this now reads "report tok/s and MLX MLX allocator peak memory". Drop the leading "MLX" on this line, for example: allocator peak memory (mlx.core.get_peak_memory()).
| `edge0-8b` (short contexts; see [Benchmark](#benchmark)). That figure is | ||
| `mlx.core.get_peak_memory()`, not process RSS — file-backed expert pages, | ||
| the MLX buffer cache, tokenizer, and interpreter are extra. The paper | ||
| reports process footprints roughly 2× the allocator peak on a 16 GB M2. |
There was a problem hiding this comment.
The rename on this bullet ("MLX allocator peak" and "not process RSS") is right. This sentence is not.
#106 is closed on a different conclusion: we report the allocator high-water mark on purpose, because RSS above it is reclaimable and does not set the machine requirement. The same build runs on a 16 GB M2.
"Roughly 2×" here sits next to the 2.9 GB figure, which was measured on a 24 GB M4 Pro. The ~2× pair in the issue is the paper's 16 GB M2 K=2/4/8 runs (allocator 2.33 / 2.60 / 3.22 GiB, process 4.91 / 5.61 / 6.39 GiB), not this row. Please remove the sentence, or scope it to those M2 runs and say the extra RSS is reclaimable.
|
Thanks for the detailed review. Pushed 2924af9:
|
shuk777
left a comment
There was a problem hiding this comment.
The two requested edits are in 2924af9.
The bench.py docstring now reads "MLX allocator peak memory" without the duplicated "MLX". The Requirements bullet drops the ~2× process-footprint sentence and says the extra RSS is reclaimable and does not set the machine requirement.
Summary
The Benchmark table and Requirements section said "peak active memory". That number comes from
mlx.core.get_peak_memory()(Metal allocator high-water mark), not process RSS. Readers took ~2.9 GB as the machine requirement; the paper's process footprints are roughly 2× the allocator peak.examples/bench.pylabels (mlx_allocator_peak=…)Test plan
test_readme_names_peak_memory_as_mlx_allocator(new hygiene guard) passesFixes #106