Lift O/D into construction and enable Chromobius on decoding server - #765
Draft
melody-ren wants to merge 20 commits into
Draft
Lift O/D into construction and enable Chromobius on decoding server#765melody-ren wants to merge 20 commits into
melody-ren wants to merge 20 commits into
Conversation
Signed-off-by: Melody Ren <melodyr@nvidia.com>
Selecting a decoder's result form by the presence of an observable matrix conflated model data with a behavior switch. The form is now chosen once when the decoder is constructed and is immutable thereafter, and O is model data only. decode() returns to being the single virtual a plugin implements. The per-call output selection, the native/derived dispatch, the capability declarations and the caller-buffer mechanism built around them are removed; a decoder that cannot produce its requested form rejects construction rather than returning the wrong shape on the first decode. Projection from an error frame to observables lives in one shared base helper, so no decoder writes its own. decoder_inputs now distinguishes a supplied observable mapping from an absent one, so a zero-row O is a model rather than a missing one, and it can canonicalize itself while retaining authoritative source and provenance. prepare_decoder_params no longer branches on decoder names, and error rates reach decoders as model data through decoder_config instead of plugin parameters. TensorRT declares its engine output format explicitly, validates a global decoder's results before indexing them, and carries that decoder's optional metadata through the combined result. Signed-off-by: Melody Ren <melodyr@nvidia.com>
The realtime path built decoder_inputs from H, O and error rates while leaving the measurement-to-detector map out, then delivered it separately through set_D_sparse. D was therefore stored twice: carried but unset on the construction inputs, and set on the decoder itself. Build D once, in GF(2)-canonical form, and hand the same matrix to both. A repeated index in a row cancels under the realtime detector XOR, so canonicalizing puts that rule in the model rather than leaving each consumer to interpret duplicates its own way. The measurement width is taken before cancellation, so a measurement referenced only by a cancelling pair still counts toward the per-decode width. set_D_sparse still performs the realtime buffer allocation, so this does not yet remove the second path; it makes the construction inputs complete. The divergence this closes is invisible end to end, since the realtime base drives decoding; it is observable only to a plugin reading its own construction inputs. The regression test therefore captures D through a decoder built by the realtime factory. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Model semantics were validated inside the YAML mapping traits, which LLVM invokes for output as well as input, so serializing a configuration also validated it. A raw Stim DEM source cannot live there: deriving its sizes needs file IO and a Stim parse, and to_yaml_str() runs on every configure_decoders() call to publish the payload. Introduce resolve_decoder_inputs(), which selects the one authoritative model source, reads and parses a DEM when stim_dem_path is set, builds the canonical measurement-to-detector map, and validates dimensions. It has no side effects, so a whole configuration can be resolved before any of it is applied. create_realtime_decoder() takes the resolved inputs rather than resolving them itself, so the model reaching a plugin is the artifact that was validated, not a second derivation of it. stim_dem_path is mutually exclusive with H_sparse, O_sparse and error_rate_vec, which are the competing representation of the same model. block_size and syndrome_size stay accepted as assertions and are verified against the values the DEM implies. The matrix branch still requires them, since the flat sparse encodings cannot be interpreted without them. Neither branch's keys can be mapRequired, so the exported JSON Schema describes the two sources as alternatives and the resolver, not the parser, decides which keys are needed. Relative model paths resolve against the directory of the configuration that named them, absolute rather than merely normalized so they keep resolving if the working directory moves. Paths are rewritten into the applied configuration only once it is in effect, so a failure cannot leave a caller's configuration partly rewritten. Applying a configuration is restaged: resolve every entry first, then construct, and stash and publish only once runtime initialization succeeds. Previously the configuration was cached and advertised before any decoder existed. Reconfiguring while a realtime session is active is rejected outright, because that session holds a reference to the decoder vector and inspects it at initialize(); callers must finalize first. A matrix configuration with no observable mapping is rejected. The decoding server constructs every decoder for observable output, and such a configuration previously produced a decoder that decoded to a zero-length observable frame. The flat detector map is validated rather than narrowed: an index that does not fit the sparse index type would otherwise alias onto a real measurement, and any value below -1 was read as a row terminator. A plugin constructor failure can still leave the decoder set empty. Avoiding that needs the old and new decoders alive simultaneously, which doubles peak decoder memory, and that cost was judged unacceptable. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Four end-to-end cases now pin the contracts the resolution work exists to provide. A matrix-source plugin stays usable both through configure_decoders and offline, with no decoder-specific framework change between them. Construction inputs agree across the two paths. The models are built independently -- the server resolves a configuration, the offline side builds decoder_inputs from the same matrices -- because reusing the handle the server produced would only prove an object equals itself. The fixture carries a repeated detector index, so the paths must agree on GF(2) collapse too; the test fails when the server stops canonicalizing, which is the class of divergence nothing observable end to end reveals. Chromobius is configured and constructed for the decoding server from a raw DEM named relative to its configuration file, loaded through the session registry rather than by calling the resolver directly. An earlier version of this test bypassed the registry and so missed that the registry resolved every model against the working directory. TensorRT nests Chromobius while the authoritative DEM survives its input derivation. Chromobius refuses to build from matrices alone, so construction succeeding is the assertion; the converse case, with the same engine and child on a matrix-only model, must fail. Also document, where the model file is read, that identifying a model by path means an in-place edit leaves the configuration unchanged and a reload keeps serving the previous model. Closing that needs the reload path to compare model content, which belongs with the transactional reload work that owns configuration comparison. Signed-off-by: Melody Ren <melodyr@nvidia.com>
The API docs still described the contract this work replaced. The PyMatching page listed O and error_rate_vec under the decoder's parameter map and said that supplying O makes the decoder return observable flips instead of an error vector. Result form is now fixed at construction and requested explicitly; supplying an observable model does not by itself change it. Both are model data, routed into the construction inputs rather than the parameter map, and PyMatching's registered schema declares only merge_strategy. The realtime configuration page showed error_rate_vec inside decoder_custom_args, which is now rejected as an unknown key: model data belongs to decoder_config alongside H_sparse, O_sparse and D_sparse. The sliding-window page had the same misplacement, and its C++ example passed error_rate_vec in the parameter map. That example no longer works: the decoder reads its priors from the model, with no parameter fallback, so it would throw on an empty rate vector. The example now builds decoder_inputs. The Python example is unaffected because the binding routes those keys into the inputs. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Resolving a DEM model went through the materialized detector_error_model, which allocates a dense detectors x mechanisms tensor that the sparse conversion then scans back out. The parser already collects per-error hit lists, and those lists are exactly H's compressed columns, so the dense form was a round trip. On a distance-13 model it cost a ~99 MiB transient to retain 4.5 MiB, and it was pure waste for a DEM-native decoder such as Chromobius, which reads the raw text and never looks at the matrices. Split the parse from the projection. dem_from_stim_text keeps its public dense contract; a library-private helper builds CSC and CSR directly into their compressed arrays. Nonzero totals are accumulated and range-checked in size_t before anything is sized or cast, so an oversized model is rejected rather than wrapping a pointer array and overrunning the index buffer. The declaration lives in a private header, not an installed one, and is explicitly hidden because this library does not set CXX_VISIBILITY_PRESET. It returns a named struct rather than a tuple: a return type is not part of a mangled symbol, so a per-translation-unit declaration could drift and still link, and H and O share a type, so positional results could be transposed while still type-checking. The equivalence test compares H and O through decoder_inputs::from_stim_dem rather than through the helper, so a projection wired incorrectly into the handle cannot pass. Separately, remove provenance_loss_reason. The invariant it decorated is worth keeping: a basis-changing derivation must drop the authoritative source, because it describes the parent's detector and error indices. derive_with_changed_basis already carries that meaning by being a distinct operation from the basis-preserving ones, so the mandatory free-form sentence added no correctness, had no production reader, and put public API around a hypothetical diagnostic consumer. A decoder that needs the raw source can say so precisely on its own behalf. The test now asserts the invariant in both directions instead of asserting the sentence. Verified with the environment documented in Building.md, after ninja install and a forced rebuild of the nvq++ sources: ctest 461/461; pytest 293 passed, 0 failed, 42 skipped. Transient allocation for a distance-13 model measured at 2.2 MiB against 4.5 MiB retained, from 99.3 MiB. Signed-off-by: Melody Ren <melodyr@nvidia.com>
decoder_inputs was already authoritative at construction, but the base still exposed set_O_sparse and set_D_sparse, so O and D could also arrive afterwards. That left two ways to supply the same model and made the contract unfalsifiable from the outside: a reader could not tell whether the new inputs eliminated late injection or merely sat beside it. Neither setter was a setter. set_O_sparse sized the corrections buffer; set_D_sparse sized the measurement buffer, the detector buffers and the streaming layer offsets, and reset the round counters. They were allocation and lifecycle disguised as assignment, re-enterable on a live decoder. The base constructor now derives all of it from the model: D and its measurement width, the measurement buffer, the detector buffers, and the corrections buffer. A decoder is usable as soon as it exists. Layer geometry is the one thing the model does not determine. It describes how a decoder consumes rounds, and the base cannot ask a subclass for it while that subclass is still being constructed, so the base previously recovered it with a dynamic_cast to sliding_window -- generic code naming a specific decoder. sliding_window now hands it over from its own constructor through a protected, construction-only initializer, guarded by a one-shot latch: without the latch, "call it only during construction" is a convention, and the mid-stream buffer reset this change removes would still be reachable. The error-frame correction path and get_num_observables read the model's O rather than a separately installed copy, so the two can no longer disagree. Every caller supplies a complete model to the factory; the setters, their hooks and the protected matrices are gone. Deleting set_O_sparse breaks the private nv-qldpc decoder, which calls it on itself. That is accepted here: this change exists to show the intended shape, and that decoder is ported separately. Verified with the environment documented in Building.md, with CUDAQ_REALTIME_ROOT set so the realtime-gated targets build, after ninja install and a forced rebuild of the nvq++ sources: ctest 496/496; pytest 293 passed, 0 failed, 42 skipped. The six PyMatchingRealtime tests that previously exercised the setters build and pass against construction- supplied inputs. Construction cost for a distance-13 model is unchanged at 4.6 MiB retained and 2.0 MiB transient. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Signed-off-by: Melody Ren <melodyr@nvidia.com>
decoder_inputs exposed three operations whose names described neither their effect nor their use: without_measurement_to_detectors() -> decoder_inputs_without_d() canonicalized() -> canonicalize_H() canonicalize_H() only canonicalizes H; O and D pass through untouched, which the old name did not suggest. derive_with_changed_basis() is removed. It read nothing from the object it was called on, so it was equivalent to constructing a fresh decoder_inputs from the caller's matrices. Sliding window now does exactly that. Dropping the raw source after a re-index is structural rather than a rule to remember: a matrix-constructed handle has no source to carry. get_default_output() -> get_output(), with default_output_ and the constructor parameters renamed to match. The output form is fixed for the lifetime of the instance; "default" implied it could be overridden later. Local names and comments for wrapped decoders now use each wrapper's own term: global decoder for trt_decoder, inner decoder for sliding_window. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Signed-off-by: Melody Ren <melodyr@nvidia.com>
Signed-off-by: Melody Ren <melodyr@nvidia.com>
The base decided between whole-block and per-round realtime behavior by reading is_sliding_window, a flag naming one concrete subclass. Replace it with round_streaming_initialized, which initialize_streaming_layout() writes after validating the layer offsets, installing the geometry and sizing the buffers. The five realtime branches now consult the same state that supplies the detector layer offsets they index, so per-round behavior cannot be selected without the geometry it requires. The flag doubles as the existing one-shot construction latch, and is written last so streaming never activates on incomplete geometry. No public API or class layout change; any decoder that installs a streaming layout gets per-round behavior, whatever its type. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Two failures, neither reproducible in a default local build. The hololink qLDPC bridge declared num_observables to size the observable matrix it now passes at construction, in a function that already had a num_observables further down. The target builds only when the DOCA and GPU-RoCE transceiver libraries are configured, so the conflict appeared first in CI. Rename the new one to num_observable_rows. Doxygen runs with WARN_AS_ERROR=FAIL_ON_WARNINGS, and the decoder constructor documented inputs but not requested_output. Verified by running Doxygen over the public headers with the project's own Doxyfile, and by syntax-checking every QEC source the local build skips. Signed-off-by: Melody Ren <melodyr@nvidia.com>
tlshannon
reviewed
Aug 6, 2026
| @@ -271,34 +343,17 @@ static void | |||
| set_sparse_from_vec(const std::vector<int64_t> &vec_in, | |||
Collaborator
There was a problem hiding this comment.
I think this function is dead code since set_O/D_sparse was removed.
tlshannon
reviewed
Aug 6, 2026
| @@ -247,10 +316,13 @@ dem_default_values dem_defaults_for_missing_keys( | |||
| static uint32_t calculate_num_msyn_per_decode( | |||
Collaborator
There was a problem hiding this comment.
I think this function is dead code since set_O/D_sparse was removed.
Adding a second configure_decoders overload made the unqualified doxygenfunction directive ambiguous, so name both signatures. The d_sparse directive qualified its parameter as cudaq::qec::sparse_binary_matrix, but the declaration sits inside that namespace and Doxygen records the argument unqualified, so breathe could not match it. Checked by generating Doxygen XML over every public header and resolving all 16 API documents against it: no unresolved directives, and no Doxygen warnings. Signed-off-by: Melody Ren <melodyr@nvidia.com>
calculate_num_msyn_per_decode(), validate_sparse_column_indices() and set_sparse_from_vec() existed only to service set_O_sparse() and set_D_sparse(). Nothing has called them since those were removed: the construction path validates and derives the same quantities from decoder_inputs. Reported by tlshannon on PR NVIDIA#765 for the first and third; the second sits between them and is dead for the same reason. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Correctness. validate_detector_rows() only rejected adjacent -1 pairs, so a leading -1 -- an empty first detector row -- reached construction and installed a decoder whose detector 0 was permanently zero. On main the same configuration failed at set_D_sparse(). Reject it, with a test that empties the first row rather than inserting one, so the row count still matches the detector count and the empty row is the only thing under test. project_errors_to_observables() documented a throw but zero-filled and returned. In-tree every caller validates at construction, but this is the extension hook for out-of-tree plugins, where silently all-zero observable corrections is the worst available failure. Make the code match the doc. Unused surface. Eight decoder::get / get_decoder overloads took a matrix or raw DEM text together with an explicit output; no caller passes anything but decoder_inputs when it names an output, and their existence is what forced the string_view disambiguation comment. Three defensive branches no in-tree caller can reach are also gone: a D row-count check decoder_inputs already enforces, a missing-D check the resolver already guarantees, and a default arm after an exhaustive switch over a two-value enum. Documentation. The plugin-authoring guide taught a one-argument base constructor and make_pcm_decoder, which this branch removed; it now matches single_error_lut_example. The walkthrough named a decoder_inputs factory that does not exist. Reported by the review pass on PR NVIDIA#765. Signed-off-by: Melody Ren <melodyr@nvidia.com>
The walkthrough was a review artifact: it opened by saying it was not in a mergeable state, and its content belongs in the pull request rather than the repository. The DecoderOutput binding was added by this branch and never consumed; the Python path selects the result basis with an output="errors"/"observables" keyword. Remove it and fold the keyword parsing, which had been copied into both get_decoder lambdas, into one helper. Also record what decoder_model_source is for: it is the entry point for a compact chunked DEM source, which would arrive as a new enumerator with its own typed constructor and accessor rather than by flattening chunks into matrices. Signed-off-by: Melody Ren <melodyr@nvidia.com>
Parallel H columns share one matching edge, so an error frame must name one of them. The column named is the one whose parameters the graph holds after the merge: KEEP_ORIGINAL and INDEPENDENT retain the first column's observables, REPLACE adopts the last, SMALLEST_WEIGHT adopts the smaller weight. Baseline main named the last column for every strategy, which contradicts the retained edge for the first two. Comment only; the behavior and its test are unchanged. Signed-off-by: Melody Ren <melodyr@nvidia.com>
…mobius-on-decoding-server
Collaborator
Author
|
/ok to test 268aa17 |
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
2 participants
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.
This PR aims to:
decoder_inputsfor O/D at construction timeBug fix
When parallel H columns share a matching edge, the error frame must name one of them. This branch names the column whose parameters the graph actually holds after the merge — KEEP_ORIGINAL and INDEPENDENT keep the first column's observables, REPLACE adopts the last, SMALLEST_WEIGHT adopts the smaller weight. Main named the last column unconditionally, which contradicts the retained edge for the first two strategies: under KEEP_ORIGINAL the matcher never uses the last column at all. Covered by PyMatchingDecoder.ErrorOutputTracksMergedParallelEdgeColumn.
Runtime / performance impact
Measured against the merge base
674cb8f2, not against an earlier commit of this branch. PyMatching throughout, three repetitions per configuration.Realtime path (the one that matters here). App and
decoding_serveras two processes over a UDP loopback wire. Run withQEC_DECODING_SERVER_SPIN_US=0so the server blocks rather than busy-polls and its CPU time measures decode plus transport rather than poll loops.At distance 9 the two are indistinguishable (751.7 / 745.0 µs against 738.3 / 746.7 µs). Both branches find the same number of corrections, so they decode identically rather than doing different work. This configuration carries its model as matrices and exercises both O and D per shot, so it is the direct test of whether lifting them out of the setters cost anything: it did not.
Resolving a model from a DEM (
H = 2184 x 47129, distance-13 surface code): parse drops from 75.5 / 97.0 / 75.2 ms to 11.7 / 11.2 / 11.3 ms, and retained memory from 105.9 MiB to ~5.9 MiB. Main materializes densedetectors x mechanismstensors for H and O; this branch builds the sparse arrays directly from the hit lists the parser already has. A Chromobius-on-the-server configuration is DEM-sourced by definition, so it pays this cost on every reload.