Repository navigation
Conversation
lil-lon
reviewed
Nov 12, 2026
Collaborator
|
@akwarii Since #642 was merged, could you rebase this onto the latest main and resolve the conflicts? I also have a few suggestions for the tests:
|
This branch has not been 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
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
With
autobatcher=True, a system that fits on the device only once or twice was refused withValueError: ... is greater than max_metric: the probe steps back to 1 copy, and the padding then puts the limit below the system's own metric. This PR makes the estimated limit safe by introducing the following changes:Never pad a limit below a system that fit. The probe of the largest system fits at least one copy of it, so every estimated limit is now at least the largest metric, after the padding. This covers
BinningAutoBatcher.load_states, both estimates ofInFlightAutoBatcher._get_first_batch(the bootstrap one included), and the estimate perf(autobatcher): reduce the number of memory probes #642 adds toInFlightAutoBatcher.load_states. Amax_memory_scalerset by the user is left as given.Keep the metrics in float64.
estimate_max_memory_scalerconverted them to float32 (torch default dtype), which could introduce rounding errors. This resulted in a system limit rounding below its own metric, and the system being refused by the autobatcher.Raise a clear error when nothing fits. When the first batch size already runs out of memory,
determine_max_batch_sizenow says that it does not fit on the device, instead of returning 1 copy that then fails with the samemax_metricerror.Closes #641. Builds on #642.
GPU validation
I reran the reproducer of #641 (MACE-MH-1+cuEq) on this branch, on a H200 GPU. Main refused these jobs with
ValueError: ... is greater than max_metricfor one of three reasons, each fixed here:max_atoms_to_try= the job's atoms), and the padding of 0.9 made the limit 0.9 × the system's own metric. Now the limit is never padded below the largest system, which the probe showed fits at least once.ts.optimizetook a first limit of 0.8 × what fits of the first system, which refused a larger second system. Now, for a job whose systems are all known, the limit is estimated over all of them (perf(autobatcher): reduce the number of memory probes #642) and kept above the largest one.autobatcher=TrueValueErrorValueErrorValueErrorValueErrorA 54,872-atom cell, which runs out of memory even alone, now gets
RuntimeError: A batch of 1 system(s) with 54872 atoms does not fit on the device.withautobatcher=True, instead of a 1-copy limit.Checklist
Before a pull request can be merged, the following items must be checked: