Skip to content

[Fix] Make the Solar model constructible and runnable - #41810

Merged
ch-wan merged 1 commit into
mainfrom
cheng/hot-fix/solar-model-construction
Sep 30, 2026
Merged

ch-wan merged 1 commit into
mainfrom
cheng/hot-fix/solar-model-construction

Conversation

@ch-wan

@ch-wan ch-wan commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

This PR is part of a stack (oldest at bottom):

Motivation

SolarForCausalLM cannot be built or run:

  • SolarModel calls make_layers without the pipeline position. In that case make_layers returns only the layer list, which is then unpacked as (start_layer, end_layer, layers), so construction fails with too many values to unpack.
  • The forward passes call self.pp_group(), which is a group rather than a function. The causal-LM forward also drops pp_proxy_tensors, so a later pipeline stage would never receive the previous stage's hidden states.
  • The logits processor is built and called with another framework's signatures: LogitsProcessor(vocab_size, org_vocab_size, logit_scale) and (lm_head, hidden_states, forward_batch).

Modifications

  • Build the layers of this pipeline stage and read self.pp_group as an attribute.
  • Pass pp_proxy_tensors through SolarForCausalLM.forward.
  • Build and call the logits processor the way other models do. Under --enable-dp-lm-head, the LM head sits on the attention-TP group, as the processor expects.
  • Solar's backbone skip connections mix hidden states saved at an earlier layer into a later one. A pipeline split that puts the two on different stages is now rejected at construction, naming the layer. test_solar_pipeline_split.py checks this against the checkpoint's skip layout: PP 1 and 2 are accepted, PP 4 is rejected.

Accuracy Tests

H200, upstage/solar-pro-preview-instruct (--trust-remote-code), GSM8K 200 questions:

main this PR
--tp-size 2 fails at construction 0.905
--tp-size 2 --pp-size 2 0.895; greedy output identical to TP2 on 4 prompts × 64 tokens
--pp-size 4 rejected: layers 16 and 48 read skip states saved on the previous stage

test/registered/unit at this PR's head, compared with main: no new failures.

Speed Tests and Profiling

Not applicable.

Checklist

Review and Merge Process

  1. Ping Merge Oncalls to start the process. See the PR Merge Process.
  2. Get approvals from CODEOWNERS and other reviewers.
  3. Trigger CI tests with comments or contact authorized users to do so.
    • Common commands include /tag-and-rerun-ci, /tag-run-ci-label, /rerun-failed-ci
  4. After green CI and required approvals, ask Merge Oncalls or people with Write permission to merge the PR.

CI States

Latest PR Test (Base): 🚫 Run #36772646799
Latest PR Test (Extra): 🚫 Run #36772646360
Latest PR Test (AMD ROCm 10): ❌ Run #36772646960

@ch-wan
ch-wan force-pushed the cheng/hot-fix/prefill-delayer-cp-gather branch from 54c6abe to defe8e8 Compare September 30, 2026 04:24
@ch-wan
ch-wan force-pushed the cheng/hot-fix/solar-model-construction branch from 3280536 to 4f20ec7 Compare September 30, 2026 04:24
@ch-wan
ch-wan force-pushed the cheng/hot-fix/prefill-delayer-cp-gather branch from defe8e8 to 3284778 Compare September 30, 2026 05:05
@ch-wan
ch-wan force-pushed the cheng/hot-fix/solar-model-construction branch from 4f20ec7 to 117af72 Compare September 30, 2026 05:05
@ch-wan
ch-wan force-pushed the cheng/hot-fix/prefill-delayer-cp-gather branch from 3284778 to 2ea8e58 Compare September 30, 2026 20:26
Base automatically changed from cheng/hot-fix/prefill-delayer-cp-gather to main September 30, 2026 20:26
`SolarForCausalLM` could not be built or run:

- `SolarModel` called `make_layers` without the pipeline position, in
  which case it returns only the layer list, and unpacked the result as
  `(start_layer, end_layer, layers)`;
- the forward passes called `self.pp_group()`, which is a group rather
  than a function, and the causal-LM forward dropped `pp_proxy_tensors`,
  so a later pipeline stage would never receive the previous stage's
  hidden states;
- the logits processor was built and called with another framework's
  signature, `LogitsProcessor(vocab_size, org_vocab_size, logit_scale)`
  and `(lm_head, hidden_states, forward_batch)`.

Build the layers of this pipeline stage, read `self.pp_group` as an
attribute, pass `pp_proxy_tensors` through, and build and call the
logits processor the way other models do, with the LM head on the
attention-TP group under `--enable-dp-lm-head` as the processor expects.
The backbone skip connections mix hidden states saved at an earlier
layer into a later one, so a pipeline split that puts the two on
different stages is now rejected, naming the layer.
@ch-wan
ch-wan force-pushed the cheng/hot-fix/solar-model-construction branch from 117af72 to 15cf773 Compare September 30, 2026 20:26
@ch-wan
ch-wan merged commit 4f3ee94 into main Sep 30, 2026
6 of 16 checks passed
@ch-wan
ch-wan deleted the cheng/hot-fix/solar-model-construction branch September 30, 2026 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant