Repository navigation
feat(memory): host-aware --max-memory on pipeline commands (#381) - #403
Merged
Merged
Conversation
Give the shared QueueMemoryOptions the same memory surface as fgumi sort: --max-memory (with `auto`), --memory-reserve, and --memory-per-thread. Under --max-memory auto the budget is detected from cgroup-aware host memory minus the reserve, so it shrinks to fit the host and pipeline commands self-throttle instead of OOM-ing on fixed-RAM hosts. The MemoryLimit/MemoryReserve types, parsers, and budget resolution move from commands::sort into commands::common so the sort buffer and pipeline-queue surfaces stay in lockstep; the legacy --queue-memory / --queue-memory-per-thread / --queue-memory-limit-mb flags remain accepted as hidden deprecated aliases. This reconciles #381 onto the #330 typed-step pipeline, preserving #330's lean single-thread queue-memory default: QueueMemoryOptions::max_memory is Option<MemoryLimit> (None = unset) so calculate_memory_limit can still return the lean 64 MiB total at --threads 1 while honoring an explicit --max-memory at any thread count. The standalone sort path and the streaming/fused sort step now call the moved resolve_memory_budget; the forced-spill path is unchanged (still reads a fixed sort buffer × threads). Also folds an Item 2 test-union residual surfaced by this change's ci-test run: restore main's group sort-order error message naming the SO:unsorted, GO:query, and SS:template-coordinate header fields (matches is_template_coordinate_sorted), so test_group_rejects_extract_output_without_sort_step passes again.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-runall #403 +/- ##
==============================================
Coverage ? 93.20%
==============================================
Files ? 138
Lines ? 54400
Branches ? 0
==============================================
Hits ? 50703
Misses ? 3697
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
added a commit
that referenced
this pull request
Jun 18, 2026
Give the shared QueueMemoryOptions the same memory surface as fgumi sort: --max-memory (with `auto`), --memory-reserve, and --memory-per-thread. Under --max-memory auto the budget is detected from cgroup-aware host memory minus the reserve, so it shrinks to fit the host and pipeline commands self-throttle instead of OOM-ing on fixed-RAM hosts. The MemoryLimit/MemoryReserve types, parsers, and budget resolution move from commands::sort into commands::common so the sort buffer and pipeline-queue surfaces stay in lockstep; the legacy --queue-memory / --queue-memory-per-thread / --queue-memory-limit-mb flags remain accepted as hidden deprecated aliases. This reconciles #381 onto the #330 typed-step pipeline, preserving #330's lean single-thread queue-memory default: QueueMemoryOptions::max_memory is Option<MemoryLimit> (None = unset) so calculate_memory_limit can still return the lean 64 MiB total at --threads 1 while honoring an explicit --max-memory at any thread count. The standalone sort path and the streaming/fused sort step now call the moved resolve_memory_budget; the forced-spill path is unchanged (still reads a fixed sort buffer × threads). Also folds an Item 2 test-union residual surfaced by this change's ci-test run: restore main's group sort-order error message naming the SO:unsorted, GO:query, and SS:template-coordinate header fields (matches is_template_coordinate_sorted), so test_group_rejects_extract_output_without_sort_step passes again.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes
--max-memoryhost-aware across all pipeline commands and reworks the memory-budget resolution so a run self-throttles to fit the host instead of OOM-ing (#380/#381).--max-memory autodetects host RAM (cgroup-aware:min(cgroup_limit, physical)), subtracts--memory-reserve, and caps the per-thread floor to what's actually available — soautonever multiplies the floor past physical memory.--memory-per-thread; both paths usechecked_muland surface a clear overflow error.QueueMemoryOptions(used bygroup,dedup,codec,clip,correct,zipper,filter,runall) withfgumi sort's--max-memorysemantics; legacy--queue-memory/--queue-memory-per-thread/--queue-memory-limit-mbremain as hidden aliases.Folded fixes
This commit absorbs two bug fixes that live in the code it introduces (squashed into this single commit; they no longer ship as standalone PRs):
MemoryLimit/MemoryReserveDisplaynow round-trips through the value-parser via binary suffixes (768MiB), so clap's shown default re-parses.--sort::memory-per-thread(reuses this commit'sresolve_memory_budgetinstead of the old inline block).Tests
80 memory/round-trip/per-thread unit tests cover: default 768 MiB/thread, single-thread fixed footprint,
autobounding by host (never oversubscribes a small host; uses the floor when the host is ample), fixed-budget independence from host, reserve shrinking, deprecated-alias behaviour, and overflow → error on every multiply path.Stack context
Part of the issue #330 unified-pipeline landing onto
feat-runall(one commit per PR). Dual-reviewed before push.