fix(algorithms): raise instead of asserting on the skipped-reference-KL pairing

[tianyi-zhang-02]

What does this PR do ?

Converts three asserts into ValueErrors so the skipped-reference-logprobs guard survives python -O.

Issues

None to close.

Usage

No config change.

Before your PR is "Ready for review"

Pre checks:

• Make sure you read and followed Contributor guidelines
• Did you write any new necessary tests?
• Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests (unit only)
• Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

ppo_train and grpo_train_sync both have:

if master_config.<algo>.skip_reference_policy_logprobs_calculation:
    assert master_config.loss_fn.reference_policy_kl_penalty == 0

python -O strips that line, and the run then trains against a KL term whose reference logprobs were never computed. With asserts enabled it still fails, but a message-less assert doesn't say which two settings conflict.

async_ppo_train already does it properly:

if master_config.loss_fn.reference_policy_kl_penalty != 0:
    raise ValueError(
        "Skipping reference logprobs requires "
        "loss_fn.reference_policy_kl_penalty=0"
    )

This is the same defect class @yuki-97 raised on #3262 — assert backend == "vllm" getting stripped under -O — which @bg51717 converted there. These two siblings were missed. All three converted sites now use the async form verbatim, so all four read alike. I swept nemo_rl/algorithms/ for other message-less asserts guarding config and found none left.

The tests run under python -O as well as normally. That is what makes them more than cosmetic: with the assert restored, both modes fail. The "supported pairing" test deliberately asserts only that the ValueError does not fire (the mocks fail later on) — otherwise it would pass vacuously if the guard were made unconditional. :)

Correction after a second pass

I had this half wrong when I opened it. grpo_train_sync's assert is shadowed: grpo.setup checks the same pairing before the train loop starts, so on GRPO the setup one is what a user actually hits — and it was an assert too, so under python -O both vanished and nothing was left. Converting only the train-loop copy would have fixed the copy nobody reaches.

The setup guard is converted now as well, and that is the one with a test.

PPO is the other way round: there is no setup-time equivalent there, so the guard in ppo_train is the only one and converting it was already right.

I kept the grpo_sync conversion too — it costs nothing and having the three sites read alike was the point.

Correction: three sites, not two, and one of them trades a message for -O safety

I under-counted in the original description. The diff converts three asserts:

• ppo.py:1229 (ppo_train) — message-less;
• grpo_sync.py:421 (grpo_train_sync) — message-less;
• grpo.py setup — this one did carry a message, naming both grpo.skip_reference_policy_logprobs_calculation and loss_fn.reference_policy_kl_penalty.

Worth being straight about the third: converting it does not improve the message — it replaces a more specific one with the shared async wording. What it buys is that the guard survives python -O, and that is the whole point, because the grpo_sync one is shadowed: grpo.setup runs first, so on GRPO the setup assert is the one a user actually hits. Leaving it as an assert would have meant converting only the copy nobody reaches.

If you would rather keep the more specific text there, it is a one-line change — say so and I will restore the message inside the raise.

[copy-pr-bot]

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.