Name the storage choices in-memory, persistent and none - #148
Merged
Merged
Conversation
zaoxing
force-pushed
the
fix/capture-fail-loudly
branch
from
September 23, 2026 23:26
ed7a0cc to
abf18c2
Compare
zaoxing
force-pushed
the
feat/storage-choice-names
branch
from
September 23, 2026 23:30
045f399 to
33d78a9
Compare
zaoxing
force-pushed
the
fix/capture-fail-loudly
branch
from
September 24, 2026 14:07
abf18c2 to
9419de0
Compare
zaoxing
force-pushed
the
feat/storage-choice-names
branch
from
September 24, 2026 14:07
33d78a9 to
90ef0e8
Compare
zaoxing
force-pushed
the
fix/capture-fail-loudly
branch
from
September 24, 2026 14:47
9419de0 to
a7cc4e3
Compare
The user's storage choice now has exactly three values: - "in-memory": records delivered in memory. Until its consumer interface exists this is the ClickHouseRecordSink path, which needs a host engine. - "persistent": the native capture storage path, object store + catalog + ClickHouse. - "none": capture off entirely. The engine allocates no ring, so none of the ring's pinned staging or payload memory. A record runtime, a host engine, an explicit ring_config and adapter attachment are refused, each with a message that names the two choices that capture. dmi.config.USER_STORAGE_CHOICES lists them. "auto" stays the unset default, the inference every caller made before the field existed; the configurator will always emit one of the three. The earlier names still work, with a DeprecationWarning that names the replacement: "native" means in-memory and "capture" means persistent. MonitoringConfig keeps whatever the caller wrote, so an integration that reads the field back still sees its own value. The engine acts on canonical_storage_backend. "none" used to mean capture and transport with no persistence: the ring still ran, and every record then failed at flush_and_wait with "record sink is not configured". Nothing depended on that, and it is now a clear refusal up front. The one test that expected an adapter to attach under "none" now expects the refusal. Docs: the v1 contract lists the three choices, the aliases and what "none" refuses; the capture-storage design doc uses the new names. Tests: a new tests/test_storage_backend_choices.py (13 cpu tests) covers the three choices, auto as the unset default, the aliases and their warning, the unknown-value message, and none allocating no ring and refusing a ring_config, a record runtime, a host engine and attachment. It failed at import before the change. The existing suites moved to the new names. CPU tier: 2238 passed, 1 skipped (no CUDA device), 0 deprecation warnings. GPU (RTX 4090) capture storage, sink ring and record-ring refusal suites: 29 passed with DeprecationWarning as an error, so no test still uses an old name. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
… the engine Two findings from an independent review: - storage_backend="none" skipped only the constructor's ring. The public enable_ring_transport(), which the adapter docs point callers at, still allocated one and made it globally active. It now refuses under "none" with the same message as create_record_runtime. - Nothing tested that a deprecated name reaches the engine: reverting the engine's mapping to the raw field left all 85 related tests green. The mapping is now a module-level canonical_storage_backend() the engine applies to whatever config it is given, so a duck-typed config carrying "native" or "capture" is checked too (before, it slipped past every check). New tests drive the engine with each old name: "native" without a host is refused, "capture" with a host is refused, "capture" with a sink config resolves to "persistent", and a duck-typed "native" is refused. With the old raw read put back, 3 of them fail. CPU tier: 2242 passed, 1 skipped (no CUDA device). GPU (RTX 4090) capture storage, sink ring, record-ring refusal and storage-choice suites: 33 passed with DeprecationWarning as an error. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
…and errors Three smaller findings from the independent review: - canonical_storage_backend returned a plain str, so every literal comparison in the engine and adapters went unchecked by a type checker. It now returns CanonicalStorageBackend, a Literal of the four names the engine acts on. - The deprecation warning used stacklevel=3, which points at the caller only for direct construction. Through dataclasses.replace() -- which the configurator's _install_schedule uses -- it was attributed to dataclasses.py. The level is now computed by walking past this module, the generated __init__ and dataclasses.py, so both routes name the caller's line. - A refusal quoted the canonical name even when the caller wrote the old one, so "native" produced an error about 'in-memory'. Refusals now read "config.storage_backend='native' (now 'in-memory') ...". Tests: three new ones in test_storage_backend_choices.py, each red first. With the storage, engine and wiring suites, 72 passed. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
The design doc's comparison table still headed its columns "native path" and "capture path", and described the persistent side as the Python reference sink, "explicitly reference-only". Since the native pack writer became the default and the in-process storage service landed, that read as "persistent = Python reference-only". The table now names both paths by their storage choice and describes the native writer and storage service as production, the Python sink as the reference and rollback. The v1 contract's list of refusals left out "none" with a ring_config (ValueError at construction) and did not say that "none" refuses create_record_runtime and enable_ring_transport with RuntimeError and an adaptor's attach_model with ConfigurationError. It now lists each with its exception type. Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
…s additions Under storage_backend="none" the adapter refusal was tested only through the private helper. A parametrized test now drives all five entry points -- the HF and base attach_model, attach_config, generate_with_monitoring and generate_greedy_with_monitoring -- and checks each raises "capture is off" before the model runs. It passed first (the behaviour was right, the coverage missing); with "none" taken out of the helper's refusal, all five fail. #146's later commits added a test and a v1 sentence that still spelled the persistent backend "capture"; both now say "persistent". Claude-Session: https://claude.ai/code/session_01PcY9QS1FAehHTzkjdpvN6Y
zaoxing
force-pushed
the
feat/storage-choice-names
branch
from
September 24, 2026 14:48
90ef0e8 to
67a07b8
Compare
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.
Stacked on #146 (which is on #143). Merge those first; GitHub then retargets this PR.
The three choices
MonitoringConfig.storage_backendis now the user's storage choice, one ofdmi.config.USER_STORAGE_CHOICES:"in-memory"ClickHouseRecordSinkand needs a host engine."persistent""none"ring_configand adapter attachment are refused, each with a message naming the two choices that capture."auto"stays the unset default. It's the inference every caller relied on before the field existed (for exampleMonitoringConfig(schedule=...), and vLLM, which passesconfig=None). It isn't a user choice; the configurator will always emit one of the three.DeprecationWarning:"native"means"in-memory"and"capture"means"persistent".MonitoringConfigkeeps what the caller wrote, so an integration reading the field back (the Megatron branches use"native") still sees its own value. The engine acts on the newcanonical_storage_backend.Behaviour change:
none"none"used to mean "capture and transport, no persistence". The ring still ran, and every record then failed atflush_and_waitwith "record sink is not configured". Now it's a clear refusal up front and the ring memory is never allocated. The one test that expected an adapter to attach under"none"now expects the refusal.Evidence
tests/test_storage_backend_choices.py(13 cpu tests): the three choices;autoas the default; the aliases and their warning; the unknown-value message listing the three choices; andnoneallocating no ring and refusing aring_config, a record runtime, a host engine and attachment. It failed at import before the change.src/still uses an old name.-W error::DeprecationWarning.nonerefuses; the capture-storage design doc uses the new names.benchmarks.mdis a dated log and keeps the names it was written with.Not in this PR: the configurator YAML
outputblock (plan milestone C2), which will emit these three values.Since the independent review (2026-09-24)
An independent review found two majors and several minors; all are fixed, test-first (58f9462..90ef0e8):
nonerefuses a ring enabled after construction too. The publicenable_ring_transport()used to allocate a ring and make it active under"none"; it now raises the same "turns capture off" error ascreate_record_runtime.canonical_storage_backend()applied to any config, including duck-typed ones, and engine-level tests drive each old name. With the mapping reverted, 3 of them fail.Literaltype (CanonicalStorageBackend).dataclasses.replace().'native' (now 'in-memory').noneis tested at all five HF entry points, not only through the helper.One review note left as a design choice: a config written with an old name does not compare equal to its new-name twin, because the config keeps what the caller wrote.
Current evidence: CPU tier 2278 passed; live ClickHouse suite 215 passed (plus the one
CREATE USERtest this local server cannot run); GPU capture, sink and refusal suites 16 passed, withDeprecationWarningas an error. CI green at 90ef0e8.