madspace: write N LHE files from combine_to_lhe, and have MadSpin use it - #86
Closed
oliviermattelaer wants to merge 3 commits into
Closed
oliviermattelaer wants to merge 3 commits into
oliviermattelaer wants to merge 3 commits into
Conversation
MadSpin's parallel unweighting wants a decay pool as one LHE file per worker, not one file it then has to stride: its _StridedEvents skips by calling next() on the other workers' events, so each of N workers parses the whole pool to read 1/N of it. It has been getting there by writing madspace's single file and splitting it in python afterwards. combine_to_lhe now takes a list of paths as well as one, and deals the events round-robin into them as it formats them, so the pool is never materialised as one file and read back. WHAT EACH FILE CARRIES All of it. The fan-out is LHEMultiFileWriter, which constructs one LHEFileWriter per path from the same LHEMeta, so every file gets its own <header> and its own <init> block with the process cross sections -- for a 1 -> n process, the channel's partial width. That is what lets a consumer open any one of them by path alone with nothing published beside it, which is the property MadSpin's owner->waiter refill handoff rests on. Tested rather than assumed (tests/test_lhe_multifile.py), down to a file that got no events still being a valid LHE file with the cross section in it. ROUND-ROBIN, NOT CONTIGUOUS Balance. read_and_combine hands out chunks of up to 1000 events, so a contiguous split of a 100k pool across 18 files would be balanced only to within a whole chunk -- 1000 events on a ~5500-event slice, and a decay pool that runs dry triggers a refill. Dealing per event is balanced to within one whatever the chunking, and it is also what the python split it replaces already did, so nothing downstream had to change. The chunks are still formatted off-thread and written back in completion order. reserve() claims a run of stream positions on the main thread before a chunk is submitted; file_index() maps each position to a file. The worker formats into one string per file and hands them over when it is done, in any order. Ordering within a file was already not guaranteed (jobs completed out of order into a single writer), so nothing downstream loses a property it had. The single-file entry point delegates to the multi-file one with a one-element list, so there is one implementation, not two that can drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing one The consumer side of the madspace change. MadSpin knows nb_core at generation time, so it can say up front how many files it wants; the run_card gets nb_output_files, mg7's counterpart of madevent's nb_unweight_output, and madevent.py turns it into the list of paths combine_to_lhe now accepts. A COUNT, NOT THE PATHS madspace takes paths -- that is the right primitive, it imposes no naming or directory policy. The run_card takes a count, and mg7 writes <run_path>/events_<i>.lhe, because the run_card is a user-facing card rendered from a template and a list of absolute paths is not a thing a user sets. mg7 names its own Events/run_NN directory, so the files then have to be moved to Events/<run_name>/unweighted_events_<i>.lhe, which is where the rest of MadSpin expects any backend's pool. That move is N renames inside one filesystem and it is not a cost worth designing around. It also buys something back: the canonical directory never contains a half-written pool, which is the same reason _materialise_refill_pool builds in a .part directory. Writing straight into it, as the python split did, did not have that. THE FALLBACK STAYS, AND STAYS EXERCISED madspace is not upgraded in lockstep with MadSpin -- it can be an older binary wheel with no multi-file overload. madevent.py detects that (LHEMultiFileWriter is the class the overload is built on), warns, and writes the single events.lhe such a build can write. MadSpin then splits it with _split_pool_round_robin exactly as it did before, so the caller sees no difference at all. Tested with a launcher stub that pretends to be the old madspace. Nothing else changes: the pool is still LHE, still at the canonical per-worker paths, so a refill still finds sources == targets and skips _materialise_refill_pool. The one other adjustment is that mg7's run directory is now identified by its info.json rather than by being the only new directory under Events/. MadSpin unit suite: 570 before, 574 after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every phase of the decay generation was missing from the report of any run
where two or more particles decay -- which is every benchmark worth running,
p p > t t~ included. decay_event_generation is the largest phase of a run and
it was simply not there, and with it decay_me_generate, decay_me_output and
the mg7 launcher phases. That left `total` as the only usable number, which is
a poor instrument for judging a change that touches one phase.
The cause is that _generate_decays forks one process per particle when there is
more than one, and the phase accounting is in-process state: the child charges
its work to a dict the parent never sees, and the dict dies with the child. The
child already marshals its results back through a small JSON file, so the
timings go with them.
Two details that are not obvious:
* the child clears the accounting it inherited through the fork before it
starts. Everything in there has already been charged in the parent, so
sending it back would double it;
* what comes back is a SUM OVER PARTICLES of work that ran CONCURRENTLY. It
is work done, not wall time, and it can exceed the wall time of the fork
that contained it. So it is charged to its own phases and never folded into
a phase measured in the parent, and a reader must not subtract it from
`total`.
The other fork points -- the unweighting shards and the max-weight scans --
have the same hole. They are left alone here: this is what the mg7 decay-pool
work needs to be measurable, not a general fix.
MadSpin unit suite: 574 before, 577 after.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oliviermattelaer
marked this pull request as ready for review
September 2, 2026 03:47
Contributor
Author
|
@theoheimel |
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.
For the madspace author. This is an optimisation, not a fix. MadSpin already
gets the outcome this asks for, in Python, with no compiled dependency, and that
fallback stays in the tree and stays exercised. Please judge it on the numbers
below, which do not obviously carry it.
The C++ change
EventGenerator::combine_to_lhegains an overload taking a list of pathsinstead of one, and deals the combined events round-robin into them as it
formats them:
The fan-out is a new
LHEMultiFileWriterinlhe_output.hpp: oneLHEFileWriterper path, all constructed from the sameLHEMeta, so everyoutput file is a complete LHE file — its own
<header>, its own<init>block with the process cross sections.
reserve()claims a run of streampositions on the main thread before a chunk is submitted,
file_index()mapseach position to a file; the worker formats into one string per file and hands
them over when it is done, in any order. The chunking, the thread pool and the
completion-order writeback are unchanged.
The single-path entry point now delegates to the list one with a one-element
list, so there is one implementation rather than two that can drift. Measured
below: that costs nothing.
Both overloads are exposed through pybind (the list caster rejects
str, sothey cannot collide), along with
LHEMultiFileWriteritself so the fan-out isunit-testable without standing up a whole
EventGenerator.Round-robin, not contiguous
read_and_combinehands out chunks of up to 1000 events. A contiguous split ofa 100k-event pool over 18 files would therefore be balanced only to within a
whole chunk — 1000 events on a ~5500-event slice — and a decay pool that runs
dry triggers a refill. Dealing per event is balanced to within one whatever the
chunking. Confirmed on the real benchmark: the 18 slices of one channel came out
16903/16903/…/16902, and every one of them carried
1.4599439254e+00in its own<init>.Ordering within a file is not guaranteed — but it already was not, since jobs
completed out of order into a single writer, so nothing downstream loses a
property it had.
Why MadSpin wants it
MadSpin's parallel unweighting gives each of N workers its own slice of a decay
pool. Handed one file instead, a worker has to stride it, and
_StridedEventsskips by calling
next()on the other workers' events — so each of 18 workersparses the whole pool to read 1/18 of it. The per-worker files also have to end
up at particular paths, because on a pool refill those paths are what the
waiting workers open.
What the alternative already in this branch does
c173250a5(in the base branch) has MadSpin write madspace's singleevents.lheand then deal it out into the per-worker files itself, with_split_pool_round_robin. Same layout, same banners, same everything — oneextra full write and full read of the pool.
That fallback is still here and still reached: madspace is not upgraded in
lockstep with MadSpin (it can be an older binary wheel), so
madevent.pycheckswhether this build has the overload, warns if not, writes the single file, and
MadSpin splits it exactly as before. There is a unit test that drives that path
with a launcher stub pretending to be the old madspace.
The path problem, and why it is a count and not paths
madspace takes paths — that is the right primitive; it imposes no naming or
directory policy. But the mg7 run_card takes a count
(
nb_output_files, mg7's counterpart of madevent'snb_unweight_output),because the run_card is a user-facing card rendered from a template and a list
of absolute paths is not something a user sets. mg7 writes
<run_path>/events_<i>.lhe, and since mg7 names its ownEvents/run_NNdirectory, MadSpin then moves them to
Events/<run_name>/unweighted_events_<i>.lhe.That move is N renames inside one filesystem. It does not erase the gain — it is
not measurable at all. It also buys something back: the canonical directory
never contains a half-written pool, which is the same reason
_materialise_refill_poolbuilds in a.partdirectory. Writing straight intoit, as the Python split did, did not have that.
The measurement
ttbar_full(p p > t t~, both tops fully decayed), spinmode PA, seed 42,nb_core = 18, 18-core Apple silicon, warm back-to-back, three passes each withthe decay directories rebuilt every time. Means of
total:Per-pass, so you can see the spread:
native 21.37 / 21.03 / 20.74
native 35.24 / 37.33 / 36.38
The phase this deletes, measured directly in the python-split runs:
decay_mg7_splitAnd the same question asked in isolation, away from MadSpin —
t > b u d~,300k events, four repetitions, madspace's own
combinewall time out ofinfo.json:_split_pool_round_robin, the same pool → 18 filesSo: the single-file path does not regress, the fan-out costs about
0.01 s, and it replaces a 0.50 s Python pass. On the benchmark that is
1.11 s off a 37.6 s run.
Verdict
Honestly: the wall-clock case is weak. 1.1 s on a 37 s run is 3%, and the
run-to-run spread of the benchmark is ±1.7 s, so the saving does not show up
reliably in
total— the two mg7 configurations overlap, and one 100k pass hadthe python split come out ahead. The measurement that does resolve it is the
phase timing, and the isolated combine bench. If a compiled dependency has to be
paid for in wall clock, this does not pay for it.
The stronger argument is not time but peak disk. The Python route
materialises the pool as one file and as the slices before deleting the
source: 276 MB per channel at 100k, two channels, so ~550 MB of transient extra
disk, and a refill adds a generation rather than replacing one. Writing the
slices directly never has both.
So: merge it if you think the primitive is worth having in madspace on its own
terms — it is a small, self-contained generalisation of an entry point that
already existed, and it makes the single-file case a special case of the general
one rather than a separate code path. Do not merge it expecting the benchmark to
move.
Also here: a measurement fix
decay_event_generation— the largest phase of a run — was missing from thereport of every run where two or more particles decay, i.e. every benchmark
worth running,
p p > t t~included._generate_decaysforks one process perparticle and the phase accounting is in-process state, so the child charged its
work to a dict the parent never saw. The child already marshals its results back
through a small JSON file; the timings now go with them. That is what makes the
decay_mg7_splitrow above exist at all — without it onlytotalwas usable,and
totalcannot resolve this change.Two things a reader has to know about those numbers: the child clears the
accounting it inherited through the fork (it has already been charged in the
parent), and what comes back is a sum over particles of concurrent work, so it
is work done, not wall time, and may exceed the wall time of the fork that
contained it. Hence
decay_event_generationreading 114% of total in a madeventrun.
Tests
madspace/tests/test_lhe_multifile.py— 11 new tests onLHEMultiFileWriter:every file's own
<init>and cross sections, the header on each, a file thatgot no events still being valid LHE, exact round-robin, balance to within one,
nothing lost or duplicated, the chunked
reserve/file_index/write_stringpath agreeing with the one-at-a-time path, one file behaving as the ordinary
single-file case, and an empty file list rejected.
Full madspace suite: 1494 pass (4 pre-existing failures need
scipy, andtest_flow/test_mlpneedtorch; neither is installed here).handshake, mg7 writing the slices itself (nothing is ever materialised as one
file), the old-madspace fallback, and the fork phase timings.
t > b u d~, 20k events,nb_output_files = 5→ five files of exactly 4000 events, each with4.8954031474e-01in its own<init>, all readable by MadSpin'slhe_parser.EventFile.One thing found on the way, not fixed here
One
cmake --buildof madspace produced alibmadspace_cpu.dylibof 2.3 MBinstead of 6.1 MB. It loaded fine and then threw
std::runtime_error: empty tensorduring survey, reproducibly, on a processthat a good build handled without complaint. Wiping
build/and reconfiguringwith the same arguments produced a library byte-for-byte the size of a
known-good one and the error went away; two further from-scratch builds since
have both been fine. So: seen once, not characterised. Flagging it only in case
it is familiar — a silently short library that fails at run time rather than at
link time is an unpleasant failure mode to hit twice.