Skip to content

feat(sc): add the teacher top-k forward for distillation on SingleController - #3843

Open
tianyi-zhang-02 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:feat-sc-teacher
Open

feat(sc): add the teacher top-k forward for distillation on SingleController#3843
tianyi-zhang-02 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
tianyi-zhang-02:feat-sc-teacher

Conversation

@tianyi-zhang-02

@tianyi-zhang-02 tianyi-zhang-02 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds the teacher top-k forward primitive needed by distillation on SingleController:

  • TQWorkerMixin.get_topk_logits_presharded runs the rank-local forward and writes both top-k tensors back to TransferQueue.
  • TQPolicy.get_topk_logits_from_meta dispatches that work from the driver.
  • The tests pin both output columns, the [B, S, k] shape, leader-only write-back, and batch-size validation.

This is the first PR in the #3843#3846#3849 stack. Nothing calls the new entry point until #3846.

Ownership

#2580 already contains the same get_topk_logits_presharded entry point and the same teacher_topk_logits / teacher_topk_indices columns as part of a larger TQ distillation implementation. That PR is still open, so ownership is unresolved. I am holding this stack here and will not add more downstream work until the maintainers choose which direction to keep.

Validation

This base branch is intentionally not being presented as current-main-ready while #2580 ownership remains unresolved; the two downstream branches are kept tested, but no further stack is being built on it.

@tianyi-zhang-02
tianyi-zhang-02 requested review from a team as code owners August 26, 2026 15:29
@copy-pr-bot

copy-pr-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@tianyi-zhang-02 tianyi-zhang-02 changed the title feat: add TQ-mediated teacher top-k forward for distillation on SingleController feat(sc): add the teacher top-k forward for distillation on SingleController Aug 26, 2026
@tianyi-zhang-02
tianyi-zhang-02 force-pushed the feat-sc-teacher branch 2 times, most recently from e29ded9 to 36323f5 Compare August 26, 2026 16:28
…troller

Distillation is the last algorithm with a rollout loop that SingleController
cannot run. Its teacher is a Policy, not a separate model class, so the
SingleController side needs no new driver -- only the missing top-k
entrypoint on the two layers every other forward already has:

  - TQWorkerMixin.get_topk_logits_presharded: per-rank fetch -> forward ->
    write-back. Unlike its siblings it writes back two tensors, and both
    carry a third axis ([B, S, k]); the write-back validates only the batch
    dimension, so that axis passes through unchanged.
  - TQPolicy.get_topk_logits_from_meta: the 1-hop dispatch, reusing
    LP_SEED_FIELDS because a teacher forward needs exactly what a logprob
    forward needs.

Nothing calls these yet -- wiring the teacher into the train pump is a
follow-up. Splitting it out keeps this piece independently testable.

Signed-off-by: Tianyi Zhang <zhangtianyi975@gmail.com>
Signed-off-by: Tianyi Zhang <123608656+tianyi-zhang-02@users.noreply.github.com>
@tianyi-zhang-02

Copy link
Copy Markdown
Contributor Author

#3768 landed while this was open, so worth saying up front how the two relate — they both put a teacher on SingleController and they are not the same thing.

#3768 (MOPD): several teachers, each on its own reserved nodes (non_colocated_teachers), routed per NeMo-Gym agent. Their logprobs become a token-level advantage via adv_estimator.name='opd'. Entrypoint is get_teacher_logprobs_presharded, config lives under on_policy_distillation.

This stack: one frozen teacher, sharing the training GPUs as a second worker group and offloaded between forwards. Its top-k logits feed DistillationLossFn as a loss. Entrypoint is get_topk_logits_presharded, config is the distillation + teacher blocks — the SC port of examples/run_distillation.py.

So: different signal, different resource model, different objective. Both entrypoints now sit next to each other in TQWorkerMixin and the two config paths reject each other explicitly (a config setting both is an error, same as ppo + on_policy_distillation).

Rebased onto #3768. One real fix fell out of it: its new validation reads algo_cfg.adv_estimator, which DistillationConfig does not have, so a distillation config died with an AttributeError inside the validator. That is in #3849.

The one thing I would like a steer on: #3768 established teacher_worker_group as an SC teacher abstraction, and this stack predates it — it reuses TQPolicy instead, since a colocated teacher is a policy that never trains and needed no new driver. If you would rather have one teacher abstraction on this path I am happy to converge on it, but the resource models are different enough (teacher_worker_group reserves nodes; this one shares the trainer's) that I did not want to force it without asking. :)

@tianyi-zhang-02

Copy link
Copy Markdown
Contributor Author

Duplicate flag, found while sweeping the open PRs. @pthombre's #2580 ("feat(distillation): add TransferQueue support for On-Policy Distillation") already adds get_topk_logits_presharded with the same signature and the same two column names, teacher_topk_logits / teacher_topk_indices. The only difference is the return value — theirs returns a transport ack, this returns None.

That PR is much larger (it also adds the write-back dispatcher, payload sizing and transport metrics), has been open since 2026-05-27, last touched 2026-06-26, and is CONFLICTING against main.

I've asked there whether it's being picked back up. If it is, I'll close this — no reason to have two. Flagging it here so a reviewer doesn't find the collision cold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-request waiting-on-maintainers Waiting on maintainers to respond

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants