Conversation
Signed-off-by: kaiyuan <kyxiezju@163.com>
Documentation build overview
46 files changed ·
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for the DAPO (Decoupled Clip and Dynamic sAmpling Policy Optimization) algorithm, adding its documentation, configuration arguments, default recipe validation, and the implementation of Soft Overlong Punishment in reward post-processing. The review feedback points out potential TypeError crashes during argument validation and rollout if rollout_max_response_len is not specified while DAPO or Soft Overlong is enabled, and suggests adding explicit checks and clearer error messages to prevent these issues.
| if args.soft_overlong_cache is None: | ||
| args.soft_overlong_cache = max(1, int(args.rollout_max_response_len) // 4) |
There was a problem hiding this comment.
If --rollout-max-response-len is not specified (i.e., it is None), calling int(args.rollout_max_response_len) will raise a TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'. Since DAPO defaults to enabling Soft Overlong, we should check if args.rollout_max_response_len is None and raise a clear ValueError to guide the user.
if args.soft_overlong_cache is None:
if args.rollout_max_response_len is None:
raise ValueError("DAPO requires --rollout-max-response-len to be set to enable Soft Overlong.")
args.soft_overlong_cache = max(1, int(args.rollout_max_response_len) // 4)| if args.soft_overlong_cache is not None and args.soft_overlong_cache < 0: | ||
| raise ValueError(f"--soft-overlong-cache must be >= 0, got {args.soft_overlong_cache}.") |
There was a problem hiding this comment.
If the user explicitly sets --soft-overlong-cache to a positive value but does not specify --rollout-max-response-len, a TypeError will occur later during rollout when calculating L_max. We should validate this condition here and raise a clear ValueError.
if args.soft_overlong_cache is not None:
if args.soft_overlong_cache < 0:
raise ValueError(f"--soft-overlong-cache must be >= 0, got {args.soft_overlong_cache}.")
if args.soft_overlong_cache > 0 and args.rollout_max_response_len is None:
raise ValueError("--rollout-max-response-len must be set when --soft-overlong-cache is enabled.")Signed-off-by: kaiyuan <kyxiezju@163.com>
GPU tests:
|
Signed-off-by: kaiyuan <kyxiezju@163.com>
Signed-off-by: kaiyuan <kyxiezju@163.com>
…ure/dapo-advantage-estimator Signed-off-by: kaiyuan <kyxiezju@163.com>
…ure/dapo-advantage-estimator Signed-off-by: kaiyuan <kyxiezju@163.com>
Signed-off-by: aoshen02 <aoshen@inferact.ai>
| if args.dynamic_sampling_filter_path is None: | ||
| args.dynamic_sampling_filter_path = ( | ||
| "vime.rollout.filter_hub.dynamic_sampling_filters.check_reward_nonzero_std" | ||
| ) |
There was a problem hiding this comment.
I think most of the ability of dapo is already support in vime, and for the soft overlong cache, I would say we recommend just use the --custom-reward-post-process-path instead of merging it, I'm still ok with merging this amount of code if it is needed.
There was a problem hiding this comment.
OK. Let's leave this as the solution for now.



Summary
--advantage-estimator dapoas a preset over the existing GRPO implementation.--soft-overlong-cacheand apply its length penalty before existing reward normalization.Design
DAPO does not add a second advantage, loss, or normalization path. Argument validation expands it to the existing GRPO primitives; the only new rollout behavior is Soft Overlong reward shaping.
Test plan
pytest tests/test_megatron_argument_validation.py(18 passed)