Repository navigation
refactor(algo): declare action_loss_type once on the config; drop finalize_group - #3845
Merged
Merged
Conversation
samsja
approved these changes
Oct 4, 2026
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.
Two small cleanups in
orchestrator/algo/. Item 4 of the orchestrator review.action_loss_typeis now declared once, on the algorithm config classes. Before, it was declared on the config classes and again on the runtime classes (algo/base.py,sft.py,opd.py,opsd.py), andbuild_algorithmasserted that the two agreed. NowAlgorithm.__init__copies it from the config, sotrain_sink'senv.algorithm.action_loss_typeis unchanged.Algorithm.finalize_groupis removed. It only calledscore_group, andtrain_sinknow callsscore_groupdirectly.finalize_episodeis kept: its "has sampled tokens" guard matters forecho, whosescore_episodedoes not filter on the mask.docs/algorithms.mdis updated to match.Behavior change: none. Every algorithm type gets the same loss routing as before, because the old assert guaranteed that the config and runtime values were equal.
Checks
pytest tests/unit/orchestrator tests/unit/test_configs* -m "not gpu": 291 passed (test_algorithmscovers the per-typeaction_loss_type).ruff check,ruff format --check: clean.GPU checks: none needed.
Conflicts
algo/base.pyandalgo/__init__.py: #3802, #3803 (small). #3838 (foldGenerationSource) also editsalgo/base.pyandalgo/__init__.py(connect_frozen_client), which needs a trivial rebase.train_sink.py:245: #3652 rewritestrain_sink.py(a one-line change there).Note
Low Risk
Refactor-only: loss routing and group scoring call paths are unchanged in effect; no auth, data, or trainer hot-path logic changes.
Overview
Single source of truth for loss routing: runtime
Algorithminstances now takeaction_loss_typefrom the algorithm config in__init__instead of duplicating it on each subclass and asserting agreement inbuild_algorithm. Per-algorithm class variables onOPD,OPSD, andSFTare removed;train_sinkstill readsenv.algorithm.action_loss_typewhen stamping samples.Thinner scoring API:
Algorithm.finalize_groupis dropped and the train sink callsscore_groupdirectly when a group has trainable survivors.finalize_episoderemains as the guarded entry point for rollout-localscore_episode(skips episodes with no trainable sampled tokens).Docs:
algorithms.mdis updated to describe this pipeline wiring and config-ownedaction_loss_type.No intended training behavior change—loss routing values are the same as before when config and runtime matched.
Reviewed by Cursor Bugbot for commit 1ebee7f. Bugbot is set up for automated code reviews on this repo. Configure here.