MILAB-6721: stop double-counting input size in the analyze memory formula - #210
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Contributor
Author
PaulNewling
force-pushed
the
MILAB-6721_mixcr-mem-formula-max
branch
from
August 4, 2026 13:09
c5399d6 to
dfc7fc7
Compare
PaulNewling
marked this pull request as ready for review
August 5, 2026 07:51
Contributor
Author
|
Merging after review from @julenmendieta . |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Sep 1, 2026
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
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.
What
The analyze memory request counted the input size twice.
baseMemGiB— 64 GiB, or 110/192 for contig/cell and MiTool presets — is a total-memory value for the whole analysis, but the formula added a data term on top of it:No
max()is needed in its place:.between(base, 256)already suppliesbaseas the lower bound. That bound was dead code until now, because an additive term can never fall below the floor it was added to.Effect
Large samples request materially less. Small samples are unchanged — they were floor-bound before and stay floor-bound. Lowering the floors themselves is deliberately out of scope; that needs separate agreement on whether 64/110/192 can safely come down.
The 256 GiB ceiling is untouched, as is the "Advanced Settings" memory override, which takes a separate
perProcessMemGBbranch this change never reaches.Structure
Two commits, kept separate on purpose:
mem-formula.lib.tengobehind a characterization test pinning today's values. Computes identical results.The formula previously sat inside
self.body(func(inputs) {...})where no test could reach it. The extraction is the seam that makes it assertable.hash_override UUID bump
mixcr-analyze.tpl.tengocarries a pinned//tengo:hash_override, bumped here to a fresh UUID. This forces a one-time recompute of results cached against the old hash.The previous value was shared byte-for-byte with
mixcr-amplicon-alignment, whose body differs substantially — and two templates pinning one UUID are interchangeable as far as the backend is concerned. Both now pin distinct fresh values; see the companion PR in that repo.Testing
pnpm exec pl-tengo testpassesTestBaseMemGiBSelectionandTestMemFormula. Each was confirmed failing before its fix, so neither is vacuous — the flip commit's test failed withexpect: 136365211648, got 254476812288until the lib changed.pnpm run build:dev-no-softwareandpnpm exec pl-tengo checkare clean; the compiled lib carries noplusand the compiled template carries the new UUID.No live run yet — this is unit- and build-level verification only. A meaningful live check needs an input the backend has never computed, or a previously-failed one. Memory and CPU go into
metaExtraand are excluded from a resource's canonical ID, specifically so dedup reuses results across differing grants, so re-running an already-successful sample cannot surface a new grant however correct the change is.Two pre-existing issues in this repo, neither introduced here and both out of scope:
pnpm run testis unusable as a gate — it chains a vitest suite needing a live backend, which fails withECONNREFUSED 127.0.0.1:6345.pnpm exec changeset statusfails onyaml.safeLoad is removed in js-yaml 4fromread-yaml-file@1.1.0, reproducible with this branch's changeset removed. The changeset was validated with the repo's own@changesets/parseinstead.Greptile Summary
This PR extracts and corrects the MiXCR analyze memory formula so input size is counted once while preserving preset floors, the 256 GiB ceiling, and explicit memory overrides. It also changes the analyze template hash to invalidate cached results and adds characterization coverage.
baseMemGiB: The total-memory floor selected from pipeline steps—64 GiB normally, 110 GiB for contig/cell assembly, and 192 GiB for MiTool. It is extracted intomem-formula.lib.tengoand used as the clamp’s lower bound instead of an additive term.memFormula: The resource formula used to request analyze RAM. It changes fromclamp(base + 4 × reads size, base, 256 GiB)toclamp(4 × reads size, base, 256 GiB).size("reads"): The stored compressed size of FASTQ inputs tagged as reads. It remains multiplied by four but is no longer combined additively with the memory floor.staticFallback: The memory used when the backend cannot evaluate the dynamic formula. It remains the selectedbaseMemGiB.perProcessMemGB: The Advanced Settings memory override. Its separate branch remains unchanged and bypasses the formula.hash_override: The analyze template’s pinned cache identity. It receives a new UUID to force one-time recomputation rather than reusing results associated with the previous formula.testscript now runspl-tengo testbefore Vitest, covering the extracted floor selection and memory arithmetic.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Analyze request] --> B{perProcessMemGB defined?} B -- Yes --> C[Use explicit GiB override] B -- No --> D[Inspect preset pipeline steps] D --> E[Select floor: 64, 110, or 192 GiB] E --> F[Compute 4 × stored reads size] F --> G[Clamp between selected floor and 256 GiB] G --> H[Submit CPU and RAM resources]Reviews (2): Last reviewed commit: "MILAB-6721: clarify memory formula comme..." | Re-trigger Greptile