Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
05e30cf to
2a974c1
Compare
|
Addressed in 2a974c1 and the description:
|
|
Follow-up from a review of how the rebase interacts with |
441ad7a to
2aa1ea4
Compare
2aa1ea4 to
e195b4f
Compare
This comment has been minimized.
This comment has been minimized.
|
/review |
This comment has been minimized.
This comment has been minimized.
|
/review |
Automated review🟡 No findings on The code hasn't changed since the last summary (head is still e195b4f). The vision-replay P1 is still fixed, and the description edit fixed the last open item: it no longer says the text model gains What the review checked
Not reviewed: Advisory, not a merge gate. Rules: |
e195b4f to
a63a63f
Compare
a63a63f to
795d283
Compare
…atterbox The harness now has a transformers reference for the Qwen3-TTS Talker and keeps the references' RoPE in fp32, so the table's "no reference for the talker" and Chatterbox's open cosine-0.65 row no longer hold, and Higgs Audio's transformers pass predates the RoPE change. Layer by layer, an adapter's median error on all three models is within about 10% of a bf16 merge's; the misses are bf16 noise below the checks' tolerance. Orpheus is described as it measured: a few misses against merged weights, none against transformers + PEFT. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sion - Qwen3-TTS's stack builds main's ResidualCodePredictor beside the Talker. - No compute scope around a forward: the pool's own plan guards deltas now. - The Qwen3.5 stack compiles without fullgraph (its per-layer cursor moves break the graph without LoRA too); the test pins that adapters add no break. - A replay's addressing is read before its commit, which clears the plan. - A text-only adapter is empty for the vision pool from its header, so its rows never wait: the test checks they take the zero slot with the pool full. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A vision prompt now reaches the LLM as its per-position ids (-1 where an image or frame row goes) plus the packed vision rows. `preprocess` embeds the ids with `embed_tokens` and splices the rows in with one masked scatter, inside the planned step, so the token embedding is an ordinary LoRA target there as on the text walks. This removes the deferred embedding: `deferred=`, `Qwen3_5LanguageModel.embed`, `embed_ids`, `_sentinel_ids`, the `all()` gate in `preprocess` and the `lora_deferred` probes. The gate skipped the embedding delta for a whole step whenever a capture bucket padded it with a dummy row (three image prompts on the bs-4 bucket), so every real row lost its adapter's delta. A padding row now carries no ids and no rows, and adds nothing. Test: test_qwen3_5_vision_prompt_text_takes_its_embedding_delta_in_a_padded_step (three image prompts plus the bucket's padding row against merged weights); fails at 3e1e06b (672/928 elements off), passes here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`Qwen3_5ForCausalLM` and `Qwen3_5VisionModel` gain `checkpoint_rules()`; the two loaders read them, and the LoRA specs take them from the roots, so adapter placement can't drift from loading. `qwen3_5/lora.py` drops its explicit `rules=` and the private `_STACKED_PARAMS` import. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop `VISION_NAMES`. No serving config spells the old names, so the vision targets take `module_lora_spec`'s default names like every other target; the docs' `targets:` example lists them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`model.embed` is a compiled region (dynamic in the patch count), and the bound patch conv's patch cutting and in-place delta are traced inside it. Inductor on CPU matches eager at 16 and 24 patches with an adapter row active. Passes at 3e1e06b too: coverage for a suspected bug that CPU Inductor does not show. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Delete Qwen3-Omni's `_thinker_remap` / `_get_thinker_stacked_params` shims (the Talker and code-predictor loaders call `thinker_remap` and `thinker_stacked_params` directly), drop Qwen3-TTS's second code-predictor filter (its rules already drop those keys), and have the test helpers load every stack with `checkpoint_rules().load`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…F12) - compare_engines sets MSTAR_REQUIRE_ACCELERATOR_GRAPHS (the old MSTAR_REQUIRE_CUDA_GRAPHS is no longer read), and an mstar server whose log has a "Failed to capture" line is refused before any cell runs, instead of printing a warning after its cells counted. - Qwen3.5 now has a prefix cache, on by default: compare_engines turns it off to match vLLM and SGLang, and phase_plan turns it off because its load repeats prompts. - prefix_cache.py and the README no longer say Qwen3.5 keys no stream. - Internal branch names (`lora/gdn`) are gone from the docstrings. - main's `fail_if_graphs_required` docstring names the variable it reads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
….F10, R1.F11) Blank lines before the "LoRA adapters" and "Python frontend" paragraphs, so RST stops merging them into the ones above. The adding_models example is OrpheusModel.get_lora_specs as written. The "Not yet" list names every registered family without LoRA specs. Higgs's docstring names its `prefill_audio` walk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1.F5) The acceptance harness's stand-in `preprocess` read `input_embeds` off a vision row; a row now brings its ids and image rows, so it splices them with the real `_vision_embeds`. The vocab-parallel embedding's collective moves from `prepare_inputs` into the step with it; the docstrings say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…(R2.F1) Qwen3.5 was the only user of a delta the node adds itself; its vision prompt now embeds inside the step, so every model either serves its embedding through the module's own forward (embeddings=True) or excludes it. Drop the parameter, the ModuleLoraMap field, the bind branch, the embeddings-check exemption, its test and its docs bullet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
….F2) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
….F3) masked_scatter_ on CUDA allocates an int64 prefix sum the size of the broadcast embeddings (8*N*H bytes; aten/src/ATen/native/cuda/IndexKernel.cpp, masked_scatter__cuda) plus a contiguous N*H mask. The rows' positions come from a cumsum and searchsorted over the 1-D mask instead: O(N) scratch, no host sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
07d5045 to
7be9c60
Compare
vasilevklart
left a comment
There was a problem hiding this comment.
Chatterbox LoRA breaks under guidance (inline), rest checks out on H200.
| def get_lora_specs(self) -> list[LoraSpec]: | ||
| """Per-request adapters on T3: every linear of its Llama backbone and the speech | ||
| head (one row per segment: each stream's last token). Under classifier-free | ||
| guidance a request's cond and uncond rows both run its adapter, as one model |
There was a problem hiding this comment.
Served with guidance on, the LoRA audio leaves the merged-weights output within the first tokens (3/3, graphs on and off); with cfg_weight: 0 it matches exactly. Looks like the uncond rows run base
There was a problem hiding this comment.
Couldn't reproduce a base uncond row, on CPU or on an H100. Step-0 max logit gap vs an fp32 merge, with guidance:
- LoRA through the pool (Triton, bf16): 0.20
- the fp32 merge itself in bf16: 0.16–0.21
- uncond rows forced to base: 0.74–0.84
Served, graphs on or off: 0.23–0.28 with guidance, 0.10–0.31 without, and greedy parts after a near-tie either way. So it looks like bf16 rounding that guidance makes about 1.5× bigger. If you still have your run, step-0 logits against an fp32 merge would settle it.
Separately, module_parity's merged checkpoint could land in the multilingual T3 when the snapshot has both files, leaving t3_cfg on base. Fixed in 33c895b.
| ctx.set_padded_rids([*rids, *pad]) | ||
| assert pool.admit(step, ctx).ok | ||
| pool.plan(step, ctx) | ||
| pool.commit(step, ctx) |
There was a problem hiding this comment.
nit: the GPU harnesses drifted from main: this fails 3/26 on H200 (a LoRA delta was called with no planned step, commit before the forward), module_parity crashes on enter_compute, prefix_cache.py on prefix_key_streams
| # leased, the captured block loop plans its own attention per replay | ||
| if (piecewise_leases or {}).get(VISION_BLOCK_LOOP): | ||
| # Leased, the captured block loop plans its own attention per replay. A node | ||
| # serving adapters still declares its frames, so the engine adds their LoRA |
There was a problem hiding this comment.
nit: no row of a leased step runs a vision adapter (the base-only region refuses them in can_lease), so these frames only give the bound patch embed and merger a plan to run base-only against
There was a problem hiding this comment.
Right, fixed the comment in aaa1639. The frames stay: without a planned step the bound patch embed and merger refuse to run.
…host reads of value-less tensors Found running mstar-project#822's test/integration/test_lora_triton_gpu.py under the emulator (which then reproduced the review's 3/26 "a LoRA delta was called with no planned step" failures on a CPU box): - header-only loading read every safetensors tensor as meta, so a test's own adapter file (its alpha scalars) lost its values; tensors under 64K elements are now read for real (a shapes-only checkpoint reads them as zeros); - torch.equal/allclose, f"{x:.3f}", .item() and friends on a value-less tensor, and host ops mixing one with real CPU tensors, read it as zeros, as every other host read does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…K.3) No row of a leased step runs a vision adapter (the base-only block loop refuses them in can_lease); the frames give the bound patch embed and merger a planned step to run base-only against, without which their deltas refuse to run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on (K.2) - test_lora_triton_gpu: bucket_step no longer commits before the forward; the capture and each replay commit after it, as the engine does (the 3 capture cases refused: a delta called with no planned step). - module_parity: the pool has no compute scope; the mixed step runs between its plan and its commit. - prefix_cache: a model keys a prefix label through KVLabel.keys in kv_labels, not prefix_key_streams. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…_labels never reads a config (K.2, review P3) keyed_labels on a bare instance crashed for models whose kv_labels read self.config (Cosmos3, V-JEPA 2) instead of exiting with the preflight's reason. Those models serve no LoRA, so the LoRA check now comes first; all 54 registered models now pass or exit cleanly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- `kill -USR1 <pid>` prints every thread's Python stack (traceback, from a thread that sigwaits; the signal is blocked before torch starts its native threads). faulthandler now dumps only the faulting thread: dumping all of them from a signal handler crashed mid-dump while they ran. - mstar_sim.pytest_plugin (`-p mstar_sim.pytest_plugin --gpu-only`): runs the tests a CPU box skips for want of a GPU, and reports an assert that fails after a read of never-computed values as an expected failure; any other failure stays one. On mstar-project#822 it leaves exactly the review's 3/26. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#823's net change (its 9 commits, d057835..f852a65) applied as one commit on lora/3-models 7be9c60, which was rebased linearly onto main. Conflicts were with main's changes: ClientStreamInput's removal, CLIENT_STREAM_ROUTE, the data worker's ResultQueue, the API server's CPU pinning, the worker's per-message spans, and slot_wait_steps' removal from the docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-worker _resource_specs) #822 made _resource_specs a list of per-worker copies, each with its own adapter-info cache. _lora_ops read it as a dict, and a load or unload now updates every copy, so a reload under the same name is checked as its new file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s its tensors The merge went into the first .safetensors that held the names, in rglob order. A full ResembleAI/chatterbox snapshot also holds the multilingual T3 under the same names, so the merge could land there and leave t3_cfg, the file the loader reads, as the base model, with no error. Every file holding a tensor is now merged, and a delta merged into none still fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#823's net change (its 9 commits, d057835..f852a65) applied as one commit on lora/3-models 7be9c60, which was rebased linearly onto main. Conflicts were with main's changes: ClientStreamInput's removal, CLIENT_STREAM_ROUTE, the data worker's ResultQueue, the API server's CPU pinning, the worker's per-message spans, and slot_wait_steps' removal from the docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-worker _resource_specs) #822 made _resource_specs a list of per-worker copies, each with its own adapter-info cache. _lora_ops read it as a dict, and a load or unload now updates every copy, so a reload under the same name is checked as its new file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#888's net change (its 19 commits, a3d22aa..e532a84, over old #823 f852a65) re-applied on the rebased #823 (lora/4-runtime-load), whose base, #822 on main, carries the reviewed #820/#821/#822. Conflicts came from three places: #822's review rounds (module_lora_spec(deferred=) gone, Qwen3.5 vision ids embedded in the step, the LoRA models loaded through their own checkpoint rules), main (per-worker _resource_specs copies, ResultQueue, CPU pinning, slot_wait_steps gone, device_loopback) and #823's fixes (LoraLoads.behind, refuse_non_adapter). Where main's reviewed LoRA code had settled a question #888 answered otherwise, #888's generalization is kept and expressed in main's semantics: - No `segment_rows` flag on nodes or region configs (main's R1.6 dropped lora_segment_rows: its one legal value said nothing). main's lora_rows becomes the generic with_segment_steps: a new step via replace() (R2.6), a step's own entry kept, none added without segments. NodeSubmodule.segment_keys replaces lora_keys; the engine's name stays engine_step. - Idle twins answer from the resource, not from request configs: Resource.idle_for (the pool's runs_adapters, inverted), so an adapter with nothing for a pool (its _empty set) leaves the step idle, as main's base-only choice did. The pool's guard against an idle step whose rows name an adapter stays. The "idle-capable spec must build request configs" engine check goes with it. - main's one_row_per_step regions captured under LoraBaseOnly are captured idle (BucketKey.idle, StepContext.idle from the bucket) and leased only to idle rows. - main's step-less warm-ups (pool.base_only) become Resource.outside_step, entered for the node's idle-capable resources. - main's module walk (stacks of any ModuleList, shape-mixed roles, unmatched `exclude`) runs inside #888's lazy declare(); _JoinedLoraMap gives a module to the node that places it before the most specific claimant, as main's linked maps did. - Request configs are built from the conductor's _node_resources() (the per-node instances), and the conductor keeps no adapter registry, so #823's per-worker spec copy fix has nothing left to update and its test goes. - Qwen3.5's leased vision step still returns None on a node without LoRA. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Based on
main. Must not merge before #1082: this is the first PR that serves LoRA under TP, and without #1082's fence a TP rank can apply a teardown between broadcast and admit and pick a different slot than the other ranks. Pending GPU testing: the rows marked "pre-rebase head" ran before the rebase onto currentmain; I'll rerun them and update the table.What this does
lm_head,embed_tokens, and the vision tower in its own pool.lm_head. An adapter on the MoE experts or router is refused.resources: lora:block in the serving YAML turns it on. Without one, every model runs exactly as before.q/k/vandunembed_tokens.docs/models.rstlists the models left out (Whisper, BAGEL, diffusion and flow models, ...) with the reason for each. It still marks Qwen3-Omni, Qwen3-TTS and Chatterbox not validated: they missmodule_parity's merged/hf rule, which is stricter than bf16 noise (see the fp32-oracle rows).How it works
get_lora_specs(module_lora_specover its weights root). Each weights root states its checkpoint rules once incheckpoint_rules(), and both the loader and the adapters use them.preprocessembeds and splices them inside the step. So the token embedding is an ordinary target, and the shared layer's deferred embedding (module_lora_spec(deferred=),lora_deferred) is deleted.module_parityand GPU harnesses followmain:Tests
ruff checkmain53c47cb, vsmainmain; 0 failing only on this branchtest_lora_models/graphs/pool+test_chatterbox_model: 229 passed, 1 skippedcfg_weight0.5cfg_weight0)test_lora_triton_gpu.py)pirate, 4B), image prompts at c=1 vs c=3benchmark.lora.prefix_cache --endpoint chat)module_paritymodule_paritymodule_paritymodule_paritymodule_parityMSTAR_PHASE_TIMING)GPU rows: torch 2.12.1+cu132, triton 3.7.1, flashinfer 0.7.2, captured and compiled as served unless noted (the Chatterbox guidance row: torch 2.11.0+cu130, triton 3.6.0, Python graph runtime). Competitor cells count only with 0 failed requests; ‡ means every round of that engine failed some, and mstar had none.
AI assistance (Claude) was used for the code, tests and this description.
🤖 Generated with Claude Code