[trainer, cfg, tests] feat: add architecture-neutral distillation trainer control plane - #545
Draft
NancyFyong wants to merge 7 commits into
Draft
[trainer, cfg, tests] feat: add architecture-neutral distillation trainer control plane#545NancyFyong wants to merge 7 commits into
NancyFyong wants to merge 7 commits into
Conversation
…iner control plane PR 1 of the distribution-matching distillation RFC (#519) delivers the architecture-neutral trainer control plane, immutable execution contracts, recipe/objective/rollout registries, and pure DMD-family math that do not depend on any model pipeline, Ray worker, FSDP model, or GPU runtime. It routes a new 'distillation' algorithm.trainer_type into a DistillationRayTrainer sibling of the existing policy-gradient/direct-preference trainers, and validates the trainer_type value in DiffusionAlgoConfig (which previously did no validation). Existing OPD behavior (distillation.enabled + policy_gradient) is unchanged and its tests still pass. AI assistance was used for this change. Co-authored-by: Claude Signed-off-by: NancyFyong <2742092809@qq.com>
… five cohesive modules The distillation package was split into 14 files that exceeded the repository's convention of grouping one responsibility per file with a registry (cf. diffusion_algos.py holding all losses + advantage estimators + registry). Merge the declarative strata: - contracts.py absorbs role_runtime/export/score_providers/checkpoint (all data-contract and validation strata). - recipes.py absorbs registry/objectives/rollout_strategies (all the declarative dispatch: registered objectives, rollout strategies, recipes). - control_plane.py absorbs phase_executor (executor protocol + CPU fakes). Net -177 lines. Public symbols are unchanged in name and behavior; import paths for every symbol now resolve through the retained modules. Clarify in math.py why it is its own module: it holds the only executable equations (contracts = data types, recipes = declarations, math = equations), so it can be unit-tested as algebraic identities without a plan/executor. No model pipeline, Ray worker, FSDP, or GPU runtime changed. All 97 PR 1 CPU tests and the full trainer/diffusion suite (334) pass. AI assistance was used for this change. Co-authored-by: Claude Signed-off-by: NancyFyong <2742092809@qq.com>
…uations.py Rename the pure-tensor-math module from math.py to equations.py. The implementation is unchanged; this is a naming fix. - math.py shadows the Python stdlib math when the package directory is on sys.path. - it carries equations (DMD gradient/surrogate, fake-score target/loss, CFG, x0 conversion, timestep shift), not generic 'math' utilities. - the single-noun name aligns with the other distillation modules (contracts, recipes, control_plane) and the visual_reflection subpackage naming. No logic changed. ruff clean; full trainer/diffusion suite (334) passes. AI assistance was used for this change. Co-authored-by: Claude Signed-off-by: NancyFyong <2742092809@qq.com>
Make the PR 1 trainer shell compatible with the production entrypoint and add an OPD-safe distribution-matching config path. Enforce immutable, fail-closed plans and transactional cycle accounting, correct causal role layouts, and align the pure CFG and x0 equations with the reviewed reference behavior. Expand CPU coverage for configuration, recipe validation, warmup transitions, failure recovery boundaries, role accounting, and numerical edge cases. Refs #519 Refs #520 AI assistance (OpenAI Codex) was used for this change. Co-authored-by: OpenAI Codex <noreply@openai.com> Signed-off-by: NancyFyong <2742092809@qq.com>
Reject fractional and boolean phase counts rather than coercing them, and require the only student phase to execute once. Keep helper definitions flat and use descriptive names across the control-plane implementation and its tests, as requested in the integrated review. Validation: 402 trainer/diffusion CPU tests and all pre-commit hooks passed. Refs #520 AI assistance (OpenAI via pi) was used for this change. Co-authored-by: OpenAI <noreply@openai.com> Signed-off-by: NancyFyong <2742092809@qq.com>
This was referenced Sep 5, 2026
Rename the controller and utility modules to match the reviewed package naming, and use descriptive snake_case state names for the new PR 1 implementation. Preserve Python protocol dunders and BaseConfig._mutable_fields because those names are framework contracts. AI assistance (OpenAI via pi) was used for this change. Co-authored-by: OpenAI <noreply@openai.com> Signed-off-by: NancyFyong <2742092809@qq.com>
8 tasks
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 does this PR do?
Draft — architecture review for PR 1 of the distribution-matching distillation stack.
Add one architecture-neutral control plane, immutable execution plans, recipe registries, pure tensor utilities, and an OPD-separated configuration seam. Related design: #519; detailed architecture and acceptance criteria: #520.
This slice deliberately does not train a model. The production constructor accepts the existing diffusion trainer interface, but model-worker initialization stops at the explicit PR 2 boundary. PR 2 adds the data plane; model adapters are subsequent work.
Reviewed head:
d7a6b99752dadcb42d7538d8e790438a7a9c7a6a. Diff: 19 files, +3,251 / −4. The companion data-plane Draft PR #546 is stacked on this head, not included here.Checklist Before Starting
[{modules}] {type}: {description}.Non-duplication: existing OPD work (#293, #498, #375, #495) concerns teacher scoring/scheduling around policy-gradient trajectories. It does not implement the student/fake-score optimizer topology or this phase-control contract. Existing fork-only review PRs are development staging, not another upstream submission.
Test
At the reviewed head, rerun before publication:
Tests cover deep immutability/pickling, recipe and role-layout validation, strict phase counts, normal/warmup schedules, skipped and failed phases, terminal recovery semantics, hook order, production constructor/config routing, OPD separation, and fp32 equation boundaries. DMD finite differences hold the stop-gradient target fixed.
CPU execution used Python 3.12 and the configured development environment. No GPU/model-execution claim is made. Core-module import tests check direct dependencies; they do not claim the repository's pre-existing package initialization imports no runtime libraries transitively. Upstream CI has not yet been established by these local results.
API and Usage Example
A complete in-process control-plane example;
synthetic-modelis a plan identifier and is not loaded:Configuration routing is
algorithm.trainer_type=distillation, with new settings underdistillation.distribution_matching. The existing OPD flagdistillation.enabledstays false. Selecting this route alone is not a runnable training recipe in PR 1.Design & Code Changes
The six-file subpackage has explicit boundaries:
verl_omni/trainer/diffusion/distillation/contracts.pyutils.pyrecipes.pycontroller.pyray_trainer.py__init__.pyKey invariants:
_mutable_fieldsremain unchanged because they are framework contracts.repeats=1, then K fake-score phases. Counts are integers, not booleans or silently truncated floats.completed_cycles, notglobal_step.global_stepcommits only after all required normal-cycle phases succeed. Each completed phase reports exactly one optimizer step for each requested role and no extras.The full implementation architecture, state transitions, tensor boundaries and extension rules are documented in #520 rather than a private/local design document.
Checklist Before Submitting
--filesavoids the local Git version's unsupportedls-files --deduplicatepath used by--all-files.*_on_cpu.pyworkflow; no GPU workflow is applicable to this slice.AI assistance: Claude and OpenAI via pi assisted implementation, review and test construction. The human submitter understands and owns the change. This PR remains Draft for upstream architecture feedback, not because it is presented as an autonomous agent submission.
Review follow-up: naming cleanup
Applied after review annotations and propagated through the whole stack (PR #8/#9). Modules and identifiers now use descriptive, repository-convention names; Python protocol dunders and framework contracts are deliberately left untouched.
control_plane.pycontroller.pyequations.pyutils.pyDistillationTrainerControlPlaneDistillationTrainerControllerbuild_control_plane()/self._control_planebuild_controller()/self.controller_instanceself._metricsself.phase_metricsself._failedself.failed_registryregistered_classesFrozenDict._dataFrozenDict.data(wrapped inMappingProxyType)_num/_batch_size/_sent/_skip_student/_fail_onnum_batches/batch_size/sent_batches/skip_student/fail_onDeliberately not renamed:
__init__,__post_init__,__all__,__slots__, and other Python protocol dunders;BaseConfig._mutable_fields(framework API required to keep config fields mutable); and inherited framework overrides such as_save_checkpoint/_load_checkpoint.