feat: Bidirectional masked importance sampling ratio (MIS) for IcePop

[casinca]

What does this PR do?

Please see more details in fixing #4735

---

Current TRL:

IcePop:

---

Note: This lower bound hparam vllm_importance_sampling_min has been made available only for MIS and not TIS.
First reason is that I'm following the original paper intentions and 2nd, unlike the upper bound utility with TIS, a lower bound would be undesirable since off policy ratios ex 0.0005 would be truncated to ex 0.5, which mean we would amplify the gradient of a bad sample. Hence the reasons I'm keeping this MIS only.

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a GitHub issue? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[casinca]

I think the logic should be good but I need some directions for the docs.

Currently the upper bound is called vllm_importance_sampling_cap or $C$ in the TRL formula, which made sense when there was only an upper bound.
I don't believe calling the lower bound "lower or min cap" grammatically correct. At the moment I've called the var vllm_importance_sampling_min but having "min" and "cap" doesn't sound intuitive, imo, for users that these are both sides of the same thing.

And if I add M to the current TRL formula:

So I'd like to harmonize and refactor vllm_importance_sampling_cap to vllm_importance_sampling_max, add a warning deprecation for backward compatibility and in the formula $M_{\min}$ and $M_{\max}$

For reference, in MiMO they call it eps_low and eps_high (but it might be confusing with asymmetric PPO clips). in IcePoP it's alpha and beta.

So not sure here what to do before proceeding further with the docs.
what do you guys think @qgallouedec @LeonEricsson ?

[LeonEricsson]

Sorry for the delay.

Looks good overall, though I’m not convinced that a lower bound is inappropriate for TIS. Intuitively, the same rationale applies on both sides: we still want to use the gradient signal even when the training policy drifts from inference policy. A lower bound would then be a way to preserve sample efficiency, with the caveat that it can amplify unreliable signal if the mismatch reflects genuinely OOD samples rather than minor drift.

Viewed this way, the motivation for double-sided boundaries applies to both TIS and MIS. TIS is essentially trying to extract information from every sample under the assumption that the inference–training mismatch is reasonably controlled, while MIS takes a more conservative stance by rejecting samples or tokens that fall outside a defined trust region.

I’ll defer the naming decision to @qgallouedec, but I agree with the concern that cap and min don’t read as a matched pair.

[casinca]

Thanks for the feedback. I will make the necessary changes for TIS as well then. With the help of the documentation, ultimately it'll give users more combinations to choose on what they want to do/act accordingly.

[LeonEricsson]

ultimately it'll give users more combinations to choose on what they want to do/act accordingly.

Agreed. We should clarify (in paper index) that the referenced TIS paper only employs an upper bound.

I'm considering extending our repertoire of importance sampling ratios with a geometric mean variant as well, but may hold of until this PR is merged

[casinca]

To update on this PR:

• added lower bound for TIS as well based on the discussion with @LeonEricsson
• took the initiative to refactor (with deprecation in mind) vllm_importance_sampling_cap to vllm_importance_sampling_max and better align with the lower min bound vllm_importance_sampling_min
• updated docs to reflect the changes
• vllm_importance_sampling_min is set to 0.0 by default for backward compatibility

I think it's good for review now, and in any case if @qgallouedec wants to rename args, the changes should be minimal now.