DMI-configurator: structured configuration, YAML, and web UI - #122
Open
zaoxing wants to merge 32 commits into
Open
DMI-configurator: structured configuration, YAML, and web UI#122zaoxing wants to merge 32 commits into
zaoxing wants to merge 32 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The implementation/documentation contract has a few concrete mismatches (e.g., config version enforcement and outdated plan snippets) plus a dependency-surface cleanup to address before safe approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds DMI-configurator, a local, no-build-step web UI and supporting Python configuration pipeline to generate a typed, validated, canonical YAML DMI capture configuration from a model descriptor plus user selections—while keeping the existing runtime configuration interface intact (YAML is a front-end serialization format + compiler path, not a runtime rewrite).
Changes:
- Introduces a structured configuration stack (
DMIConfigschema, YAML parse/dump, validation, compilation, compatibility bridge). - Adds a FastAPI backend + static frontend UI (packaged assets) and a
dmiCLI forui+describe-model. - Adds a new runtime-adjacent selection filter (
filter_by_layers) to support layer-range authored configs.
File summaries
| File | Description |
|---|---|
| tests/test_configurator_api.py | Contract tests for configurator HTTP API + static asset serving. |
| tests/test_configuration_yaml.py | YAML canonicalization + round-trip + golden-file coverage. |
| tests/test_configuration_validation.py | Model-aware validation tests (availability, layer bounds, schedule checks). |
| tests/test_configuration_schema.py | Unit tests for schema dataclasses (LayerSelection, ModelTopology, policies). |
| tests/test_configuration_manifest.py | Descriptor parsing/IO tests and ModelShape translation checks. |
| tests/test_configuration_introspect.py | Tests for deriving descriptors from HF-style configs and rejection cases. |
| tests/test_configuration_compatibility.py | Tests for legacy selection bridge + compilation + new layer filtering. |
| tests/golden/qwen3-moe.yaml | Golden config artifact for MoE model case. |
| tests/golden/qwen3-basic.yaml | Golden config artifact for minimal/basic config case. |
| tests/golden/qwen3-attention.yaml | Golden config artifact for attention-over-range config case. |
| tests/data/moe-decoder.model.yaml | Synthetic MoE descriptor fixture for availability testing. |
| src/dmi/ui/static/styles.css | New UI styling (light/dark, layout, state chips, issue rendering). |
| src/dmi/ui/static/index.html | New single-page UI shell and controls layout. |
| src/dmi/ui/static/architecture.js | SVG architecture renderer driven by server-provided metadata. |
| src/dmi/ui/static/app.js | UI state management + server round-trips for validate/parse/serialize/save. |
| src/dmi/ui/server.py | Uvicorn entrypoint wrapper for serving configurator. |
| src/dmi/ui/app.py | FastAPI app + endpoints delegating to configuration layer; static mounting. |
| src/dmi/ui/init.py | Lazy imports so dmi.ui doesn’t require FastAPI at import time. |
| src/dmi/hooks/selection.py | Adds hook_belongs_to_layers + filter_by_layers (additive runtime filter). |
| src/dmi/configuration/yaml.py | YAML parse/dump + canonicalization + disk IO helpers. |
| src/dmi/configuration/validation.py | Validation producing field-addressed issues for UI display. |
| src/dmi/configuration/schema.py | Canonical typed configuration + descriptor schema definitions. |
| src/dmi/configuration/manifest.py | Descriptor parsing/IO and translation to DMI ModelShapeConfig. |
| src/dmi/configuration/introspect.py | Descriptor derivation from HF configs/model ids with lazy transformers import. |
| src/dmi/configuration/errors.py | Configuration-layer exception types. |
| src/dmi/configuration/compiler.py | Compiles DMIConfig to CompiledDMIConfig via existing selector + layer filter. |
| src/dmi/configuration/compatibility.py | Bridge between structured observations and legacy selection string. |
| src/dmi/configuration/catalog_adapter.py | UI-facing hook metadata projection + availability/reason computation. |
| src/dmi/configuration/architecture.py | Defines diagram nodes and produces model/layout payloads for the UI. |
| src/dmi/configuration/init.py | Consolidated public API exports for configuration subsystem. |
| src/dmi/cli.py | Adds dmi ui and dmi describe-model CLI commands. |
| pyproject.toml | Adds [ui] optional deps, dmi console script, and UI static package data. |
| examples/model_descriptors/README.md | Documentation for descriptors + how to generate/use them. |
| examples/model_descriptors/llama3-8b.yaml | Shipped example descriptor used by tests/UI. |
| docs/dmi-configurator-plan.md | Design/implementation plan and rationale for configurator architecture. |
Review details
Suppressed comments (2)
docs/dmi-configurator-plan.md:244
- The CompiledDMIConfig snippet documents a
runtime: RuntimeConfigfield, but the implementedCompiledDMIConfig(src/dmi/configuration/compiler.py) only carrieshook_specs,schedule, and optionalpolicy. Keeping the snippet aligned avoids implying runtime transport settings are part of config compilation.
@dataclass
class CompiledDMIConfig:
hook_specs: list[HookSpec]
schedule: CaptureSchedule
runtime: RuntimeConfig
policy: RuntimePolicy | None
docs/dmi-configurator-plan.md:255
- The
compile_configexample callsfilter_by_layers(specs, config.observations.layers), but the implemented function takes explicitstart/endints (and compilation also no longer passes aruntimefield). Update the snippet so readers can copy/paste it without hitting a TypeError or referencing non-existent fields.
def compile_config(config, model_context):
specs = select_hook_specs(
model_context.specs,
",".join(config.observations.hooks),
cfg=model_context.shape,
)
specs = filter_by_layers(specs, config.observations.layers)
return CompiledDMIConfig(
- Files reviewed: 35/35 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The backend save endpoint and YAML parser currently mishandle error/type cases (uncaught ConfigurationError in /api/config/save and or {} / or [] defaulting that can silently bypass structural validation).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 48/48 changed files
- Comments generated: 3
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are additive, well-covered by CPU-marked tests across schema/YAML/validation/UI API surfaces, and the runtime layer-range wiring is exercised end-to-end.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pyproject.toml:20
- Optional:
tests/test_configurator_api.pyimportsfastapi.testclient.TestClient, which depends onhttpxin many FastAPI/Starlette setups. If someone installs only the[ui]extra, the UI will work but running the API contract tests may fail with anImportErrorifhttpxisn’t pulled transitively; consider addinghttpxto theuiextra (or adding an explicitimportorskip("httpx")guard in the tests) to make the optional stack more self-contained for contributors.
- Files reviewed: 48/48 changed files
- Comments generated: 0 new
- Review effort level: Lite
XbzOnGit
reviewed
Sep 2, 2026
Collaborator
Author
|
All 14 inline findings are addressed at dd3bb32 (responses on each thread). Two notes: the vLLM layer-range fix (3919326699) needs a matching revision of the pinned third_party/vllm-integration submodule — in this repo the failure is now a diagnosable error rather than a silent range drop. The 6 Copilot findings were already fixed at c8875a8/de37754 and are pinned as regression tests in tests/test_pr122_review_findings.py. CPU suite at head: 1373 passed. |
Samfisheryu
requested changes
Sep 5, 2026
Samfisheryu
left a comment
Samfisheryu
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes for confirmed runtime and estimator correctness issues. I reproduced each finding inline. The existing CPU suite is healthy (1,387 passed), and the supported Hugging Face torch.compile schedule guard itself behaves correctly; the blocking gaps are configuration application, adapter ownership, and packed/vLLM parity.
Collaborator
Author
|
Independent review round: 8 adversarial reviewers, all confirmed findings fixed at 3dc68ca. Review lenses: config pipeline, HTTP security/correctness, schedule-gate runtime, estimator fidelity, frontend JS, descriptor/schema integrity, test honesty (mutation-tested), PR-body coherence. Every finding was verified by execution before being flagged; every confirmed one is fixed with a test in this commit. Fixed:
Known ceilings (disclosed, deferred): the vLLM integration's direct commit path is not schedule-gated (separate repo, DMI-vLLM-Integration#21 adds the layers keyword; gating is next); torch.compile/CUDA-graph replay bakes the gate at first trace (compiled decode should stay on default schedules until enforcement moves native). CPU suite: 1414 passed. |
Samfisheryu
requested changes
Sep 5, 2026
Samfisheryu
left a comment
Samfisheryu
left a comment
Collaborator
There was a problem hiding this comment.
Thanks — I independently re-ran the original repros on 27c40b3. Several fixes are substantive (live layer rejection, design-time labelling, save serialization, ordinary duplicate-key and hook-type validation, port handling, and clean CLI errors). Keeping changes requested for the still-incomplete runtime capability boundary and the confirmed residual implementation bugs below.
Collaborator
Author
|
All 8 follow-up findings are fixed at 450b340 (replies on each thread), with regression tests in tests/test_pr122_round4_review_findings.py. CPU suite: 1679 passed; both CI checks green. Two known ceilings remain disclosed in code rather than fixed here: the vLLM direct-commit path still bypasses the schedule gate (needs the graph-safe integration change, separate repo), and the token-range index records attempted rather than captured traffic. |
Collaborator
Author
|
All review threads on this PR are now resolved (43/43: 6 Copilot, 14 XbzOnGit, 23 Samfisheryu), each with its fix commit, mechanism, and pinning test recorded on the thread. Since the last review round: packed estimator honesty, the verbatim vLLM partition rule, one-owner attach enforcement, fail-closed encoder subtypes, ring/workload/YAML boundary hardening, offset/warmup disclosure, plus the CUDA-graph schedule disclosure, CLI error narrowing, and selection taxonomy. Suite: 1679 passed; both CI checks green on 450b340. Requesting re-review. |
zaoxing
added a commit
that referenced
this pull request
Sep 6, 2026
The review round surfaced three rules that deserve a written rationale rather than a thread of replies: the configuration artifact is strict end-to-end; the runtime surface is enforced at the smallest possible layer (the adapter driver's before_forward) rather than scattered across every emit point; and the HTTP surface trusts the loopback bind but refuses the network without a per-launch X-DMI-Token. The one-owner attach invariant is the same rule applied to a model. The alternatives weighed (stricter loopback defaults, TLS/OAuth on the local tool, rejecting packed configs that name a schedule, hooking the gate at every emit point, an inline JSON or frontmatter idiom) all get a sentence here so the next round does not relitigate them.
zaoxing
added a commit
to ProjectDMX/DMI-vLLM-Integration
that referenced
this pull request
Sep 6, 2026
* Accept a layer range in VLLMAdaptor.attach_model DMI-configurator (ProjectDMX/DMI#122) forwards a selected layer range as attach_model(model, hook_selection, layers=LayerSelection(...)). The adaptor refused any ranged configuration with TypeError, and Packed/vLLM is the configurator's default backend. The keyword is applied in _VLLMHookSelection.from_model so the filter hits BOTH sides: the installed local specs and the model-wide candidate-rank formula sets. A signature-only fix would let the per-rank byte plans disagree with what the filtered local specs actually capture. Global hooks (layer_no < 0) are never restricted, matching the HF adapter. A range that matches nothing raises a DMI ConfigValidationError naming the range instead of silently compiling to an empty selection. Closes #20. Covered by tests/test_attach_layer_range.py, which runs the selection logic on CPU behind a vLLM import stub, so CI needs no vLLM install; the six existing suites that need the real release were already failing on this machine before this change (no vllm distribution) and are unaffected. * Keep the adapter importable and attachable on DMI without the layer-range API The official-vLLM CI matrix runs this repo against released DMI (v1.1.0) and DMI main, neither of which exports hook_belongs_to_layers -- the module-level import broke collection of every test module there. The layer-range API imports now live inside the layers branch (with a clear upgrade-DMI error when a range is passed without them), and attach_model forwards the keyword to super() only when a range is configured, so un-ranged attaches behave exactly as before on old DMI. The new tests skip cleanly where dmi.configuration is absent and pin the old-DMI contract by simulating the missing facade name. * Stub vLLM in tests only when vLLM is not installed The old guard (sys.modules + hasattr distributed) skipped the stub only when vLLM was already imported; an installed-but-unimported vLLM got shadowed for the whole session, so other tests' view of vllm depended on collection order. importlib.util.find_spec decides on installation, not import state: real vLLM always wins, the stub only exists for CPU CI where vLLM is absent. The suite's assertions are behavior-based on the DMI side and hold under either vLLM. Review thread: copilot on PR #21 (test_attach_layer_range.py:23). --------- Co-authored-by: Alan Liu <zaoxing@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
BackendAdapter.before_forward() does not advance the step counter when build_step_context() returns None, which can break warmup/stride accounting and capture steps more often than the configured schedule.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 63/63 changed files
- Comments generated: 2
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The public integration API docs currently mis-specify the filter_by_layers signature/return type, which can mislead downstream integrators even if runtime behavior is correct.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 63/63 changed files
- Comments generated: 3
- Review effort level: Lite
…slation, doc snippet
Blocking: - Non-loopback binds now mint a per-launch token demanded on every mutating endpoint (the file-writing API is no longer open to the network). - Save refreshes the server's reload state, so a refresh restores what was saved and a second save cannot overwrite it with the launch config. - Save runs model-aware validation before the write; invalid configurations get a 400 naming the issues instead of a successful persist. - The capture schedule is enforced: the adapter driver consults should_capture_step/should_capture_request before every step (StepContext carries the phase; HF reports it), so capture_decode=false actually stops decode capture and strides thin the capture. - compile_config refuses requested hook types absent from the model's spec list, so "valid" means executable (pos_embed/mlp_post on a Llama can no longer compile to an empty selection). - attach_config diagnoses an adapter that cannot accept `layers` with a ConfigurationError naming the remedy (pinned vLLM integration update is a separate-repo follow-up). Major: - YAML boundary is type-strict: exact ints (no 2.9-truncation, no bools), real booleans (no "false" strings), unknown keys in layers/policy refused. - Descriptor topology requires exact positive integers; head_dim is validated; num_layers 1.5 is refused at the boundary instead of exploding in range(). - Introspect refuses known non-decoder model families (BERT, ViT, T5, ...) instead of labeling every non-encoder-decoder config decoder_transformer. - The frontend preserves policy absence when loading policy-less YAML. - Copy serializes the current state instead of the possibly-stale preview. - Estimator volume figures sum every rank (TP/PP splitting no longer deflates bytes_per_request); peak stays per-rank for ring sizing. - Estimator dtype accounting matches the runtime: packed token/topk ids are int32, topk weights float32; batched token ids int64. - Workload gains cache_max_len so StaticCache prefill attention can be sized; without it the estimate says which cache it assumes. CPU suite: 1373 passed.
…Packed/vLLM Copilot's third pass on PR 122 noted that fastapi.testclient needs httpx, which the [ui] extra does not carry. With Starlette 1.6 the import raises RuntimeError (it wants httpx2 now) at collection, so the whole API contract suite errored instead of skipping like every other optional-dependency suite. Both TestClient-using modules now skip unless httpx2 or httpx is importable; the transport stays out of [ui] on purpose (the UI never makes HTTP requests) and pyproject says so. A child-process test pins the skip-not-error behaviour. Review finding 3919326699: the pinned vLLM integration's attach_model has no `layers` keyword, so attach_config refuses a ranged configuration on that backend at launch. The estimator -- where the UI learns which backend is targeted -- now warns about this on Packed (vLLM) with a layer range, and the attach_config error links the tracking issue (ProjectDMX/DMI-vLLM-Integration#20). CPU suite: 1372 passed; the 5 remaining failures are the /proc/self/fd fsync tests, which are Linux-only and identical to main.
The high-severity one: refusing a step in before_forward only skipped plan/commit -- the model's HookPoints still dispatched producers during that step's forward, writing unreserved bytes into the ring and desyncing the task/meta FIFO so later captured records were misattributed or dropped. The driver now disarms the transport (capture_step=False) before refusing and re-arms it on a committed step; HookPoint.forward checks the flag first and returns without dispatching. CPU-level test drives a real HookPoint through both flag states with the producer call recorded. - Sustained rate and per-day volume now divide by request_stride as well (the driver's request gate drops whole requests, matching per_request). - The estimator states its enforcement assumption when strides or phase flags are configured: an adapter reporting no phase / non-numeric request ids applies only part of the schedule. - Architecture refusal matches real HF spellings: "BertForMaskedLM" (no underscores) now trips the masked-LM check; unknown-key TypeError guard for head_dim covers bools exactly. - Nits: constant-time token comparison in the UI middleware, isdecimal for scheduler-id gating, and the never-fires compiler check now runs after selection (shape suppression) but before layer filtering so its message cannot blame the layer range for an absent hook. CPU suite: 1377 passed.
Companion to 30f0e8a, which committed only four of the six files before a concurrent stash took the rest of the working tree. This carries the core of the fix: the transport gains a capture_step flag that the driver disarms before refusing a schedule-refused step and re-arms on a committed one, and HookPoint.forward checks it before dispatching -- so refused steps no longer launch unreserved producers or desync the task/meta FIFO. Also lands here: request_stride in the sustained rate, the estimator's enforcement assumption, the BertForMaskedLM spelling fix, and the regression tests for all of it. CPU suite: 1385 passed.
DMI-vLLM-Integration#21 accepts attach_model(..., layers=LayerSelection(...)), and it imports everything through dmi.api.v1 — which did not carry the layer-range pieces. filter_by_layers, hook_belongs_to_layers, and LayerSelection are now facade exports alongside the rest of the selection API, the exactness test and integration-api-v1.md cover them, and a pin test locks the three to their real implementations. The submodule pin bump and the estimator's vLLM layer-range warning removal land together after DMI-vLLM-Integration#21 merges.
…ing-fit task cap, gate disclosures Eight independent reviewers audited this PR; every confirmed finding is addressed here. HTTP boundary (was raw 500s on malformed input): - /api/config/parse rejects a non-string yaml value with 400. - /api/estimate: Workload requires exact ints/finite floats (float 8192.0 exploded as TypeError in the byte arithmetic; bools read as 1), the ring block requires integer payload/pinned bytes and OverflowError is caught, and pinned_bytes without payload_bytes is a 400 instead of a silent no-op. Non-ASCII X-DMI-Token headers compare as bytes (was TypeError -> 500) and /api/config is token-gated on network binds (it names a server path). - The loopback bind refuses mutating requests carrying a FOREIGN Origin: TrustedHostMiddleware stops DNS rebinding, not plain CSRF -- a web page can send Host: 127.0.0.1 legitimately, and sendBeacon can send application/json without a preflight. Foreign Origin is the one signal a browser always gives. Data safety: - save_config/save_descriptor write to a temp file and os.replace: a mid-write ENOSPC used to destroy the previous configuration. - The descriptor parser now matches the config parser's strictness: exact-integer schema_version (true/1.0 are refused), unknown keys refused in the model section and at the root, a bad model.id raises DescriptorError instead of bare ValueError, and num_experts without top_k is refused (the routing observations could never fire). Estimator fidelity: - check_ring_fit gains task_entries: prepare_step refuses on the task ring too, so a wide full-preset model "fit" on bytes while every real step returned OVERSIZED. /api/estimate accepts ring.task_entries and names the actual breach in the detail text. - The PP layer split now matches vLLM's get_pp_indices (remainder on the FIRST stages), so the peak rank is the rank the runtime actually pressures on non-divisible layer counts. - Batched final_logits counts logits_to_keep=1 (what HF generate() materializes) instead of every prefill row -- the old number was ~1000x the reality and set the ring-fit verdict. Schedule gate: - The no-phase tail (warmup/stride for adapters that report no phase) is now pinned by tests, the docstring documents it and the request-gate units, and an unrecognized phase value captures-as-unreported instead of crashing the driver and silently disarming all later steps. - catalog_adapter marks final_logits unavailable when vocab_size is 0 (compute_hook_shape returns [] for it; validation/estimate/compiler all certified it before). Frontend: - Input handlers guard on an initialized flag: listeners bind before the model fetch resolves, and a keystroke in that window crashed on null state (and a mid-applyState crash discarded a loaded file). - Copy carries a stale-response stamp like the other async paths. - Tabs expose aria-selected and panels are role=tabpanel. Docs: capture-policy-review's "schedule enforced by no shipped adapter" finding is marked resolved (it landed in this PR after the doc was written); the PR body no longer claims the change is additive-only nor lists #121 as an open follow-up; the pyproject httpx comment matches reality (it is a core dependency). CPU suite: 1414 passed.
c00afa4, while adding pyyaml/fastapi to the CPU job's install, removed the entire clickhouse-live job (its services, its fetch-depth: 0, its skip gate) from python-checks.yml — so the manual ClickHouse suites this branch carries ran on nobody's CI again, which is the exact regression that job was created to prevent. The failure only surfaced once the configuration pipeline started importing yaml at module level: the live job's install never had pyyaml, and collection of the new suites died with ModuleNotFoundError. The job is restored from main verbatim, with pyyaml and the [ui] deps added to its install so the configurator suites collect and run there too. Verified locally against the CI selection: 30 collected, 0 skipped.
…stimates, fail-closed detection Every finding was reproduced as a test before any source changed (tests/test_pr122_round3_review_findings.py). The review was written against c19eba5; 3dc68ca had already closed three of the fifteen, and those stay in the suite as regression pins rather than being dropped. Blockers: - attach_config now installs the configuration's CaptureSchedule on adapter.engine.config, which is the only place _schedule_allows reads it. Before, the documented path (load_config -> attach_config) applied WHERE to capture and silently dropped WHEN: a YAML declaring capture_prefill: false or step_stride: 17 attached hooks and captured everything. An adapter with no engine still attaches under the default schedule and raises for one that asks for anything, matching the rule the `layers` keyword already follows. - Attachment has one owner. BackendAdapter.attach_model records the owning adapter on the model and HuggingFaceAdapter.detach_model releases only its own marker; generate_with_monitoring (and the greedy loop, same hazard) refuses a model somebody else attached instead of building a second adapter over the shared transport -- which left the forward with one caller's reservation and this call's producers, then detached the first attachment on the way out. - Packed estimates no longer divide by step_stride/request_stride. The pinned vLLM integrations build MonitoringEngine(config=None) and call commit_step directly, so no schedule reaches the gate and production captures every step; dividing reported a reduction the shipped runtime does not perform, on the UI's default convention. The figures now say so. Batched (Hugging Face) still divides -- that driver does gate. Correctness and honesty: - compile_config refuses a layer range that removes every spec of a selected hook, naming the live layer span. A stale descriptor claiming 32 layers against a model exposing 16 used to compile layers 20-25 to an empty spec list that installed nothing and reported nothing. - Architecture detection fails closed: an allowlist of causal decoder families replaces the non-decoder blacklist, which could only refuse what someone had thought to list (dinov2, modernbert and bert-generation all escaped it and were emitted as decoder_transformer). A config with no model_type is refused too -- geometry that parses says nothing about causality. Hand-written descriptors remain the escape hatch and skip detection entirely. - /api/validate and /api/config/save label their verdict design-time and name the descriptor as its authority; the UI's "ready to run" wording follows. Validation cannot certify runtime-readiness from a portable descriptor: a decoder Llama exposes no pos_embed spec however the descriptor marks it. - A strided estimate is disclosed as a long-run average bounded by the peak step, and offsets/warmups warn that they shift which steps are captured rather than being silently absent from the arithmetic. Boundaries: - Saves serialize the write and the state update under one lock, so a 200 names one committed revision. Racing saves used to leave the file and the served config disagreeing until restart. - /api/config/parse and load_config share a loader that rejects duplicate mapping keys: PyYAML's last-wins would let a merge conflict change capture scope in silence. - observations.hooks is a typed list[str] -- [q, 1, true, null] no longer becomes ["q","1","True","None"]. - Workload requires a real bool for packed ("false" was truthy and read as the vLLM convention) and a non-bool number for the decode rate. - An explicit --port outside 1-65535 is refused before the descriptor is read; port 0 used to advertise a URL naming no bound port. Two findings are answered with disclosure rather than the reviewer's first option, both stated in the estimate output: VLLM_PP_LAYER_PARTITION on the serving host overrides the partition this estimator cannot see, and the per-request figure is exposed as a long-run average instead of evaluating the predicates for a stated request window. Existing tests that pinned stride division on packed workloads were split by convention rather than deleted, and the packed counter-case is pinned alongside them. CPU suite: 1473 passed. The 5 remaining failures are pre-existing and Linux-only (capture-storage fsync tests read /proc/self/fd), verified failing identically on a pristine 3dc68ca.
…rrowing, selection taxonomy - A non-default capture schedule now disables CUDA-graph compilation for the generate() call, mirroring the capacity-overflow path: the gate is a Python branch in HookPoint.forward and replay bakes it at the first decode trace, so a refusal (or capture) at step one would apply to every later step. Default schedules are unaffected. Pinned by driving generate_with_monitoring with a stubbed adapter and inspecting the kwargs the model receives. - cli.main no longer catches bare RuntimeError -- that class carries genuine bugs (RecursionError is one) and swallowed them into one stderr line. The optional-dependency failure gets its own UIDependencyError; internal raise sites updated. - compile_config re-raises an unknown hook selection as ConfigurationError instead of bare ValueError, so callers funneling on the taxonomy (the UI's 400 mapping) see a 400, not a 500. CPU suite: 1588 passed (after the main merge).
…hip, boundaries Samfisheryu's follow-up review kept CHANGES_REQUESTED on eight residuals; all are fixed here with tests, RED first. Ownership: - attach_config refuses a second adapter on an already-owned model BEFORE any mutation (no schedule install, no attach call), naming the current owner; same-adapter reconfiguration is allowed; detach by a non-owner leaves the marker (HF adapter path pinned). Packed honesty (the vLLM runtime executes no schedule): - capture_prefill/capture_decode no longer shrink packed volume, peak, or sustained figures -- the runtime captures both phases regardless. The authored intent stays visible as an explicit warning; batched figures are unchanged. - vLLM PP partition rule ported from 0.27.1's get_pp_indices (remainder backward from the second-to-last stage, never the last; cited cases [1,2,1], [2,3,3,2], 32/40 verified) plus an explicit, validated pp_layer_counts override for VLLM_PP_LAYER_PARTITION deployments. - Offset/warmup disclosure no longer depends on a stride being set. Boundaries: - Ring: only absent/None defaults pinned_bytes; every explicit value is validated (exact non-bool int, >= 0), and check_ring_fit refuses negatives itself. - Workload.cache_max_len requires an exact integer. - YAML: unhashable mapping keys raise ConstructorError (a 400), and the descriptor file path uses the same strict loader as configs. - Introspect: any model_type containing "encoder" fails closed instead of riding a decoder family prefix (qwen2_audio_encoder); decoder+head configs still accepted. CPU suite: 1679 passed.
- The per-request token-range index records every driven step including schedule-refused ones; it is the attempted-traffic index, not the captured-traffic index. - The _accepts_layers probe cannot distinguish forwarding **kwargs from tolerating them; the backstop is the v1 contract plus compile-time live-hook rejection, stated where the probe lives.
Attention-weight hooks (pattern, attn_scores) attend at most window KV positions on sliding-window models, so they are shaped against min(context, window) instead of the full context. Only kv_dim is affected, and only those shapes consume it. Exact-int >= 1 validation, assumption text when set, no-op when absent or above context. FP8 KV-cache quantization was investigated and deliberately not modeled: hooks capture forward activations in model dtype, so cache-store quantization changes zero hook payload bytes.
…mates" The knob's core semantic was incorrect in the unsafe direction: compute_hook_shape returns full [heads, q_len, kv_dim] for pattern/attn_scores with no window input, plan_step reserves exactly that, and these hooks fire meaningfully only in eager mode -- where SDPA materializes the full scores matrix and masks it rather than shrinking it. Capping kv_dim by the window made the estimate report less than the runtime reserves (up to context/window under-count), and check_ring_fit would bless a ring the runtime overflows. The FP8 non-modeling analysis in the original message stands; the window half is withdrawn until a backend materializes band-shaped scores.
…y banner - Save disables itself for the request round trip (re-armed in finally). - Layer rail ticks are keyboard-operable checkboxes sharing one picking function with the click path; architecture nodes already had this. - Estimate panel marks itself loading with aria-busy, cleared on either guarded outcome. - Status chip is a polite live region; ring meter exposes value semantics. - Token-gated binds get a persistent curl-only banner revealed on 401 instead of per-panel curl-hint errors.
- _reject_unknown gains an error= kwarg so the config half (ConfigurationError) and the descriptor half (DescriptorError) share one body. As a bonus this stops the int-YAML-key AttributeError the descriptor half had -- the manifest copy already used repr() but the config half still did not. - save_config and save_descriptor share _write_text_atomic(path, text, what, error): the only delta is which exception class wraps an OSError. CPU suite: 1685 passed.
The review round surfaced three rules that deserve a written rationale rather than a thread of replies: the configuration artifact is strict end-to-end; the runtime surface is enforced at the smallest possible layer (the adapter driver's before_forward) rather than scattered across every emit point; and the HTTP surface trusts the loopback bind but refuses the network without a per-launch X-DMI-Token. The one-owner attach invariant is the same rule applied to a model. The alternatives weighed (stricter loopback defaults, TLS/OAuth on the local tool, rejecting packed configs that name a schedule, hooking the gate at every emit point, an inline JSON or frontmatter idiom) all get a sentence here so the next round does not relitigate them.
The yaml.py/manifest.py simplification shipped first without its test updates, which broke CI: the old atomic-save test raised ENOSPC from dump_config, and the new save_config evaluates dump_config before _write_text_atomic's try-block, so the OSError escaped unwrapped. The test now patches Path.write_text on the .tmp sibling instead, which is the real I/O boundary the atomic write protects. Rest of the round: - UI_DEPENDENCY_MESSAGE moves to errors.py beside UIDependencyError, so app.py and server.py quote one string. - The auth banner reuses .notice plus one margin-top override instead of duplicating seven properties. - Copy calls a shared serializeCurrentState() rather than reissuing the /api/config/serialize POST shape refreshOutput already builds. - The two test-side brace matchers (_function_body / _plain_function_body) collapse into one with an optional async prefix. CPU suite: 1685 passed.
DMI-vLLM-Integration PR #21 merged (squash 23717cb): attach_model accepts layers=LayerSelection(...), applies it to the local specs and the model-wide candidate-rank sets, and stays importable on DMI builds without the layer-range facade. The pin moves old-main 29f26c3 -> 23717cb, picking up both the V2-runner promotion (#19) and the layer range (#21).
The warning told the UI that attach_config refuses a ranged configuration on Packed (vLLM) until the integration accepted ranges -- that is PR #21, now merged and pinned. The estimator test flips to pin the absence of the warning (RED against the stale block), and attach_config's fail-closed guard stays for genuinely older integration checkouts, with its comment updated to name them. CPU suite: 1689 passed.
…vLLM The recipe for proving a saved .dmi.yaml artifact drives a real vLLM 0.27.1 model through dmi.configuration.attach_config existed only in the verification session: build the CI-style env (vllm 0.27.1 + DMI --no-deps + integration --no-deps), subclass the V1 DMXGPUWorker to suppress the self-attach (V2 refuses subclasses), run the engine core in-process, and assert ownership, ranged spec layers, disabled out-of-range hook points, and a completed generate. Also correct the install step for shared checkouts: the native build emits ABI-suffixed extensions per interpreter, so 'make clean' is destructive there.
zaoxing
force-pushed
the
feat/dmi-configurator
branch
from
September 7, 2026 20:04
a2ed118 to
4def7cc
Compare
dmi ui/describe-model needed transformers installed by hand to resolve a model directory or bare HF model id; pip install -e ".[hf]" makes that reproducible, and the missing-transformers error now names it.
Keep the top-level entry point consistent with the configurator plan doc.
Samfisheryu
reviewed
Sep 8, 2026
Samfisheryu
left a comment
Samfisheryu
left a comment
Collaborator
There was a problem hiding this comment.
Re-review at 393f4f3: 354 tests passed across the eight relevant configuration/runtime/estimator/API regression suites. The pinned vLLM adapter now accepts and forwards layers.
One confirmed regression remains beyond the already disclosed feature limitations: the previously fixed None-context step accounting and its tests are absent from this head. I added the old/new execution results to the existing inline thread: the author's original four tests pass on 74d7be7, but three fail on 393f4f3. Please restore that fix; the prior "fixed" reply does not describe the current tree.
The vLLM direct-commit schedule gap and attempted-vs-captured token-range behavior remain acknowledged limitations, not newly reported implementation bugs. This recheck does not claim a full vLLM/HF graph or multi-GPU run.
Two conflicts, both where main's #127 (native capture write path) and this branch added to the same block; both resolved as the union, since the two sides configure unrelated things: - .github/workflows/python-checks.yml: main added the botocore/boto3 pip lines the native capture suites need at collection time; this branch added pyyaml and the [ui] extra (fastapi/uvicorn/httpx). Both jobs need both sets. Keeping only one side would trip main's own new gate, which fails the build when a cpu test skips for anything but absent hardware -- the configurator's API contract suite skips itself without fastapi/httpx, and the native suites error at collection without botocore. - Makefile: main added PYTEST_ARGS (so CI can pass --junit-xml to the gate); this branch added MODEL for the `make ui` target. Independent variables in the same header block. No src/ file was touched by both sides, so nothing else conflicted.
Samfisheryu
reviewed
Sep 10, 2026
Samfisheryu
left a comment
Samfisheryu
left a comment
Collaborator
There was a problem hiding this comment.
Full re-review at badbfca: no new reproduced implementation blocker beyond the previously acknowledged runtime schedule limitations. The restored None-context step-accounting fix is retained and its regressions pass; the pinned vLLM adapter accepts and forwards layers.
Validation: 2,038 CPU tests passed; 16 real single-GPU native/reference/ClickHouse Ring tests passed; GPT-2 HF eager and CUDA Graph correctness E2Es passed (2 tests, 56 subtests). Both CI checks are green. These E2Es validate their tested/default capture paths, not unsupported schedule enforcement.
Keeping the existing requested changes for the core capability boundary: vLLM's direct commit_step path still bypasses schedule gating, and non-default HF schedules still disable compilation instead of implementing graph-safe enforcement. The attempted-vs-captured token-range behavior also remains acknowledged. I am not treating documentation nits as blockers; this run does not claim full vLLM or multi-GPU E2E coverage.
The /polish loop's working files are per-run agent bookkeeping, not project artifacts: nothing in the tree, the build, or CI reads them, and they go stale once the branch they describe has landed. #133 removes them from main; dropping them here too so a merge of this branch cannot carry them back in.
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 this is
DMI-configurator: a local web tool that turns a model descriptor plus a few visual selections into a validated DMI capture configuration.
One job:
The architecture visualization is the interaction surface; the generated YAML is the product. This is a configuration tool, not an observability dashboard.
See
docs/dmi-configurator-plan.mdfor the full design, its verification against the codebase, and the decisions taken.The central architectural decision
DMI does not switch from Python configuration to YAML. YAML is added as a serialization format in front of the existing mechanisms, via a loader/compiler pair:
The separation is the point. What is explicitly rejected:
yaml.safe_load()producing a dict that every module then indexes into. One parser, one typed object, everything else downstream.The frontend holds no DMI semantics — it derives its choices from the hook catalog through a backend adapter, and every validity answer is a server round-trip through the same Python the runtime uses. What the UI calls valid is what DMI calls valid.
What landed
dmi/configuration/schema.pydmi/configuration/manifest.pydmi/configuration/catalog_adapter.pydmi/configuration/architecture.pydmi/configuration/validation.pydmi/configuration/yaml.pydmi/configuration/compatibility.pydmi/configuration/compiler.pydmi/configuration/estimate.py(+POST /api/estimate)dmi/hooks/selection.py(filter_by_layers)dmi/ui/app.py,dmi/ui/server.pydmi/ui/static/(no build step)dmi/configuration/introspect.pydmi/cli.pydmi/adapters/base.py,dmi/adapters/types.py,dmi/hooks/point.py,dmi/transport/ring.pyPhases 1–7 of the plan, plus #121's runtime wiring (layer ranges applied at attach, estimator, launch ergonomics) and capture-schedule enforcement in the adapter driver. Phase 8 (runtime policy) is deferred by design.
Compatibility
The public API-v1 facade grows three exports (
filter_by_layers,hook_belongs_to_layers,LayerSelection);filter_by_layers/hook_belongs_to_layersfollow the same convention asfilter_by_pp_rank/filter_by_tp_rank.attach_modelgained a keyword-onlylayers=...parameter on the base class and the HF adapter — call sites that never pass it are unaffected, andattach_configdiagnoses an adapter that cannot accept it rather than crashing.The capture hot path does change behavior, beyond the additive filter:
StepContextgained aphasefield the HF adapter reports,before_forwardnow consults the capture schedule before every step (phase flags, strides, warmup — previously authored and ignored), the transport disarms producers on schedule-refused steps (a driver-level skip alone left HookPoints writing unreserved bytes), andHookPoint.forwardhonors that flag. Steps driven outsidebefore_forward(e.g. the pinned vLLM integration's direct commit path) are not schedule-gated yet; the estimator discloses what its reductions assume.The flat
dmx_hook_selectionform stays supported through a compatibility adapter rather than being made canonical.pyyamlandpydanticwere already core dependencies;fastapianduvicornare new behind a small[ui]extra. A normal DMI install pulls in no web framework.Deliberate constraints
runtimeblock in the YAML. The existing runtime surface isRingConfig, a native struct of transport parameters. Exposing a subset would imply a support contract that does not exist.layers.endis inclusive.LayerSelection(8, 15)selects eight layers; the UI labels it "Layers 8–15" and the label must not lie.decoder_transformer. Encoder-decoder configs and known non-decoder families (BERT/ViT/T5-style) are refused rather than mis-rendered.Verification
pytest -m cpu: 1414 passed (descriptor, configuration, serialization round-trip, integration, golden-file suites, plus the schedule-gate and API-contract suites).parse(serialize(config)) == configafter canonical normalization — what makes the YAML a configuration artifact rather than a one-way export.🤖 Generated with Claude Code (review round included 8 independent adversarial reviewers; all confirmed findings fixed on this branch)