🐌 Clean two-sided clipping #3499
Conversation
tests/test_grpo_trainer.py
Outdated
There was a problem hiding this comment.
It might seem risky to remove these tests, but having tests that are so tightly coupled to internal implementation details (like calls to _get_log_prob) makes future development extremly difficult. Even if it's less secured, I recommend manually inspecting intermediate results instead, and keeping unit tests focused on what they're meant for: validating external behavior.
There was a problem hiding this comment.
Ah sorry this was my fault for asking to test the new functionality. I agree with you that we can remove these tests, with the comment that the current ones only test for a change in the weight, not whether the clipping is actually behaving as expected. I suppose those thing either need to be exposed as public methods or part of integration tests where we can access the metrics to validate the values are clipped accordingy
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| else: | ||
| # Original GRPO clipping (only lower bound implicitly applied by the final min) | ||
| per_token_loss1 = coef_1 * advantages.unsqueeze(1) | ||
| coef_1 = torch.clamp(coef_1, max=self.args.delta) |
tests/test_grpo_trainer.py
Outdated
There was a problem hiding this comment.
Ah sorry this was my fault for asking to test the new functionality. I agree with you that we can remove these tests, with the comment that the current ones only test for a change in the weight, not whether the clipping is actually behaving as expected. I suppose those thing either need to be exposed as public methods or part of integration tests where we can access the metrics to validate the values are clipped accordingy
|
i can try to integrate into liger too |
|
Yes, I completely understand the motivation behind these tests. Ideally, we should have this kind of test that checks if the loss is well calculated with a few reference values, but considering the current implementation, it requires patching and all, so it's not ideal. If in the future we split the |
Some late review on #3434