[refactor] add generic LossConfig.weight, drop ListwiseRankLoss.alpha - #676
Conversation
ListwiseRankLoss.alpha was a loss-mixing weight rather than a hyperparameter of the InfoNCE objective: temperature_init and learnable_temperature reach the loss module's constructor, alpha never did -- it was a scalar multiply at the call site. It existed only because the weight ladder stopped at the task level, so no other loss could be reweighted at all: a task carrying both a point-wise and a focal loss had no knob, and a SID model summed its reconstruction, commitment and contrastive terms 1:1:1. Add the missing rung as LossConfig.weight (default 1.0, mirroring EasyRec's Loss.weight) and honor it at the single return tail of each of the three _loss_impl bodies, which every loss() loop routes through. The listwise branch no longer returns early; it nulls the per-candidate loss_weight that does not apply to it and falls through to the shared tail, which also stops MultiTaskRank's task weight from being silently dropped for that loss. Configs that still set alpha now fail to parse, which is preferable to the silent 10x stronger list-wise term a default change from 0.1 to 1.0 would otherwise cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QBwVb17wQxFd5Kdb8mkwPS
692b9e7 to
bb934ef
Compare
| @@ -53,8 +57,6 @@ message BinaryCrossEntropy { | |||
| // prediction (carry it next to a logit loss such as | |||
| // binary_cross_entropy); currently the DlrmHSTU family satisfies both. | |||
| message ListwiseRankLoss { | |||
There was a problem hiding this comment.
Consider reserving the removed field's number and name:
| message ListwiseRankLoss { | |
| message ListwiseRankLoss { | |
| reserved 1; | |
| reserved "alpha"; |
The repo already does this in train.proto (reserved 4, 16, 17; plus the names). The risk today is low — configs are persisted text-only (config_util.save_message → MessageToString), so no binary artifact can reinterpret field 1 — but reserving is zero-cost and prevents a future field from silently reusing the number, and prevents alpha from being reintroduced with a different meaning (a name the sibling JRCLoss / BinaryFocalLoss messages already use for something else).
| and the total is | ||
| ``sum_k task_weight_k * sum_l loss_weight_kl * L_kl``. |
There was a problem hiding this comment.
Minor: this composition formula isn't actually asserted. Every task here runs at the default task weight 1.0 — _task_configs only exposes a weight knob on is_like, which carries no listwise loss — so the task_weight × LossConfig.weight product is unpinned anywhere (the loss.md claim 它与任务级的weight相乘生效 also leans on it). Risk is low: both factors are individually pinned and the composition is two plain multiplies (dlrm_hstu.py:304 over the shared _loss_impl tail). But since this test is the natural place for it, consider building base/scaled with a non-1.0 weight on the is_click task and asserting the product on listwise_rank_loss_is_click — or soften the docstring to what's asserted.
| # The sid loss modules are stateless, so both configs can score the | ||
| # same predictions without matching the models' parameters. |
There was a problem hiding this comment.
Nit: true for the two modules this test uses — SidReconLoss/SidCommitmentLoss are pure functions with no parameters/buffers — but SidContrastiveLoss has learnable logit_scale_* parameters, so "the sid loss modules are stateless" overstates the invariant the trick relies on. Extending this two-models/one-prediction pattern to a contrastive config would compare module-owned temperatures across models. Suggest narrowing the comment, e.g. "The recon/commitment loss modules are stateless, so ...".
Review summaryRan a five-area review (code quality, performance, test coverage, documentation accuracy, security/robustness) over the diff. Overall this is a clean, well-scoped refactor: the missing rung in the weight ladder is added at exactly the right level, and the migration story is sound. LGTM with three minor inline comments (proto Verified across the change:
Two non-blocking notes:
|
Motivation
ListwiseRankLoss.alphawas documented as "weight of the list-wise term relative to the task's other losses" — a loss-mixing weight, not a hyperparameter of the InfoNCE objective. The tell is in the code:temperature_init/learnable_temperaturereach theListwiseRankLossmodule's constructor, whilealphanever did — it was a scalar multiply at the call site inRankModel._loss_impl. (BinaryFocalLoss.alphaandJRCLoss.alphaare module constructor args and stay untouched.)It existed only because the weight ladder stopped at the task level.
task_towers.weight/task_configs.weightscale every loss of a task alike, so they cannot trade one loss of a task against another — the test that pinnedalphasaid so in its own docstring. The consequence was that no other loss could be reweighted at all: a task carryingbinary_cross_entropy+binary_focal_losshad no knob, and a SID model summed its reconstruction, commitment and contrastive terms 1:1:1.What this does
Adds the missing rung,
LossConfig.weight(default 1.0), mirroring EasyRec'sLoss.weight, and deletesListwiseRankLoss.alpha:The weight is honored at the single return tail of each of the three
_loss_implbodies —RankModel,MatchModel,BaseSidModel— which all sevenloss()loops route through, so no consumer can silently ignore it. Two smaller decisions:listwise_rank_lossbranch no longer returns early. It returned early to skip the per-candidateloss_weight, which is sized off the candidate count rather than the request count; it now nullsloss_weightand falls through to the shared tail. That also stopsMultiTaskRank's task weight (folded into the tensorloss_weight) from being silently dropped for this loss — unreachable today, since onlyDlrmHSTUpublishes the candidate counts and applies task weight in its ownloss(), but the trap is gone.weight == 1.0, so every existing config produces an identical traced graph.The reported per-loss value is the weighted one, as
alphawas, which keeps the invariant that the logged terms sum tototal_loss.Compatibility
A config that still sets
alphanow fails at load with a protobufParseErrornaming the field. This is deliberate: silently accepting it would move the effective default from 0.1 to 1.0, i.e. a 10x stronger list-wise term that would surface days later as a training-dynamics regression rather than as a config error. The fix is mechanical —alpha: 0.1becomesweight: 0.1, one nesting level out. No in-repo.configcarries it.Test Plan
test_loss_weight_scales_lossinrank_model_test.py, over NORMAL and FX_TRACE graphs with and without sample weights. It reuses the exact batch and expected values oftest_binary_classification_model, so a weight folded into the sample weights instead — which would renormalize away — would not reproduce them; the FX rows confirm the Python float constant-folds.test_loss_weight_scales_lossinmatch_model_test.pyandtest_loss_weight_scales_only_its_own_terminsid_rqvae_test.py, covering the other two_loss_implbodies. The SID one also pins that the weight touches its own term and not its sibling.test_listwise_loss_is_scaled_by_alpharenamed totest_listwise_loss_is_scaled_by_loss_weight; its docstring was the one place that documented whyalphaexisted, so it now documents whyLossConfig.weightdoes.tzrec/models(238) andtzrec/loss(43) suites green on an A10, including the GPU-gateddlrm_hstu_onerank_test(18).pre-commit runandpyrefly checkclean.Docs
docs/source/models/loss.mdgains a损失权重 weightsection (how it composes with the task-level weight, that the logged value is weighted, and a caution against tuning it alongsideuse_pareto_loss_weight) and alistwise_rank_losssection, which was missing entirely; its stale supported-loss list is refreshed.dlrm_hstu_onerank.md's example and parameter table move the knob out one level.Alternatives considered
A learnable per-loss weight (EasyRec's
learn_loss_weight) was rejected —pe_mtl_lossalready covers adaptive weighting across tasks. Putting the knob onFusionSubTaskConfigwas rejected as the wrong granularity: it stays OneRank-specific and leaves every other multi-loss task unserved.🤖 Generated with Claude Code
https://claude.ai/code/session_01QBwVb17wQxFd5Kdb8mkwPS