Skip to content

[feat] Add multi-turn format checking reward managers with refactored… - #4593

Closed
albertimff wants to merge 1 commit into
verl-project:release/v0.6.1from
albertimff:releasev0.6.1_multiturn_format_check_reward_manager
Closed

[feat] Add multi-turn format checking reward managers with refactored…#4593
albertimff wants to merge 1 commit into
verl-project:release/v0.6.1from
albertimff:releasev0.6.1_multiturn_format_check_reward_manager

Conversation

@albertimff

Copy link
Copy Markdown

… utilities

Files Modified:

  • verl/experimental/agent_loop/tool_agent_loop.py: Enhanced to record full message history for format reward computation
  • verl/experimental/reward/reward_loop/init.py: Added exports for new format check reward managers
  • verl/workers/reward_manager/init.py: Added exports for new format check reward managers

Files Added:

  • verl/experimental/reward/reward_loop/format_check_dapo.py: DAPO reward manager with format checking
  • verl/experimental/reward/reward_loop/format_check_naive.py: Naive reward manager with format checking
  • verl/utils/format_reward.py: Core format reward computation utilities with helper functions
  • verl/workers/reward_manager/format_check_dapo.py: Worker implementation for DAPO format checking
  • verl/workers/reward_manager/format_check_naive.py: Worker implementation for naive format checking

Purpose of Changes:

The format reward check manager was rewritten to address multi-turn conversation scenarios where the previous implementation would concatenate all subsequent assistant/tool messages, causing single-turn format checks to become ineffective.

The new solution extracts all assistant messages from the complete conversation history and performs individual format validation on each turn, ensuring proper format checking regardless of conversation length.

Implementation Details:

  • tool_agent_loop.py: Modified to always record assistant messages and pass full message context via extra_fields
  • format_reward.py: Provides extensible format validation with regex patterns for thinking/tool/answer tags, plus refactored utility functions:
    • compute_format_reward(): Core format validation logic
    • apply_format_reward_to_score(): Helper for scalar reward scores
    • apply_format_reward_to_tensor(): Helper for tensor reward processing
  • Reward Managers: Refactored to use shared utility functions, reducing code duplication
  • Both DAPO and naive reward manager architectures supported
  • Full message history preservation enables per-turn format analysis

Key Features:

  • Full message history preservation in agent loop for per-turn analysis
  • Configurable format checking logic that users can customize
  • Refactored utility functions for better code reuse
  • Support for both DAPO and naive reward manager architectures
  • No breaking API changes - fully backward compatible

What does this PR do?

Add concise overview of what this PR aims to achieve or accomplish. Reference related GitHub issues and PRs that help with the review.

Checklist Before Starting

  • Search for similar PRs. Paste at least one query link here: ...
  • Format the PR title as [{modules}] {type}: {description} (This will be checked by the CI)
    • {modules} include fsdp, megatron, sglang, vllm, rollout, trainer, ci, training_utils, recipe, hardware, deployment, ray, worker, single_controller, misc, perf, model, algo, env, tool, ckpt, doc, data, cfg, reward
    • If this PR involves multiple modules, separate them with , like [megatron, fsdp, doc]
    • {type} is in feat, fix, refactor, chore, test
    • If this PR breaks any API (CLI arguments, config, function signature, etc.), add [BREAKING] to the beginning of the title.
    • Example: [BREAKING][fsdp, megatron] feat: dynamic batching

Test

For changes that can not be tested by CI (e.g., algorithm implementation, new model support), validate by experiment(s) and show results like training curve plots, evaluation results, etc.

API and Usage Example

Demonstrate how the API changes if any, and provide usage example(s) if possible.

# Add code snippet or script demonstrating how to use this

Design & Code Changes

Demonstrate the high-level design if this PR is complex, and list the specific changes.

Checklist Before Submitting

Important

Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.

… utilities

## Files Modified:
- verl/experimental/agent_loop/tool_agent_loop.py: Enhanced to record full message history for format reward computation
- verl/experimental/reward/reward_loop/__init__.py: Added exports for new format check reward managers
- verl/workers/reward_manager/__init__.py: Added exports for new format check reward managers

## Files Added:
- verl/experimental/reward/reward_loop/format_check_dapo.py: DAPO reward manager with format checking
- verl/experimental/reward/reward_loop/format_check_naive.py: Naive reward manager with format checking
- verl/utils/format_reward.py: Core format reward computation utilities with helper functions
- verl/workers/reward_manager/format_check_dapo.py: Worker implementation for DAPO format checking
- verl/workers/reward_manager/format_check_naive.py: Worker implementation for naive format checking

## Purpose of Changes:
The format reward check manager was rewritten to address multi-turn conversation scenarios where the previous implementation would concatenate all subsequent assistant/tool messages, causing single-turn format checks to become ineffective.

The new solution extracts all assistant messages from the complete conversation history and performs individual format validation on each turn, ensuring proper format checking regardless of conversation length.

## Implementation Details:
- **tool_agent_loop.py**: Modified to always record assistant messages and pass full message context via extra_fields
- **format_reward.py**: Provides extensible format validation with regex patterns for thinking/tool/answer tags, plus refactored utility functions:
  - `compute_format_reward()`: Core format validation logic
  - `apply_format_reward_to_score()`: Helper for scalar reward scores
  - `apply_format_reward_to_tensor()`: Helper for tensor reward processing
- **Reward Managers**: Refactored to use shared utility functions, reducing code duplication
- Both DAPO and naive reward manager architectures supported
- Full message history preservation enables per-turn format analysis

## Key Features:
- Full message history preservation in agent loop for per-turn analysis
- Configurable format checking logic that users can customize
- Refactored utility functions for better code reuse
- Support for both DAPO and naive reward manager architectures
- No breaking API changes - fully backward compatible

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a robust mechanism for format checking in multi-turn conversations by refactoring the reward managers. The core logic is encapsulated in a new format_reward.py utility, which is then used by new DAPO and naive reward managers. The agent loop is also updated to ensure the full message history is available for this per-turn analysis. My review focuses on the correctness and robustness of the new format checking logic. I've identified a potential issue where leading or trailing whitespace in assistant messages could lead to incorrect format validation, and I've provided a suggestion to improve robustness.

for idx, content in enumerate(assistant_contents):
is_last = idx == len(assistant_contents) - 1
expected_pattern = answer_pattern if is_last else tool_pattern
if not expected_pattern.match(content):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation for format validation using expected_pattern.match(content) is sensitive to any leading or trailing whitespace in the assistant's message content. Language models can sometimes generate extraneous whitespace, which would cause an otherwise correctly formatted message to fail validation and receive an incorrect penalty. This could negatively impact the training process by providing wrong reward signals.

To improve the robustness of the format check, I recommend stripping whitespace from the content before applying the regular expression match.

Suggested change
if not expected_pattern.match(content):
if not expected_pattern.match(content.strip()):

@albertimff albertimff closed this Dec 18, 2025
gaohongkui pushed a commit to gaohongkui/verl that referenced this pull request Aug 8, 2026
…tron-core 0.18.x

## What does this PR do?

Adds a compatibility shim so Multi-Token Prediction can be used together with
`recompute_granularity=full` on every released megatron-core, and makes the existing
signature-based branch in `patch_mtp_layer_checkpointed_forward` visible instead of silent.

### The bug

megatron-core 0.18.x is internally inconsistent: `MultiTokenPredictionLayer.forward` calls

    self._checkpointed_forward(..., padding_mask=padding_mask)   # multi_token_prediction.py

while `_checkpointed_forward` itself does not declare `padding_mask`. Any MTP run with
`recompute_granularity == 'full'` therefore dies at the first forward:

    TypeError: MultiTokenPredictionLayer._checkpointed_forward() got an unexpected
               keyword argument 'padding_mask'

Upstream cause: two landings that do not compose -- NVIDIA/Megatron-LM#2645 added the
call-site kwarg, verl-project#4593 refactored the method without it. Tracked as
NVIDIA/Megatron-LM#4933 (open since 2026-05-22).

It is fixed on megatron-core `main`, but `core_v0.18.2` was tagged 2026-07-20 -- two months
after that issue was filed -- and is still the newest release, so **every released
megatron-core hits this**. verl users cannot currently combine MTP with full activation
recomputation without patching megatron themselves.

### Why the existing patch does not cover it

`patch_mtp_layer_checkpointed_forward` skips any layer whose `_checkpointed_forward` does not
start with `forward_func`. That gate matches megatron-core 0.14-0.17 only; 0.18+ renamed the
first parameter to `hidden_states`, so on every 0.18+ install the patch silently does nothing:
when `target_layers` is non-empty but `patched_count == 0`, neither log line is printed, so
there is no way to tell whether it applied. Skipping is in fact *correct* on 0.18+ (megatron
now keeps non-tensor args out of the checkpoint natively, via its own `custom_forward`
closure) -- but it should say so.

## Changes

- `_patch_padding_mask_kwarg()`: when the layer's `_checkpointed_forward` lacks `padding_mask`,
  rebind it to accept and drop the kwarg. `padding_mask` is dropped rather than forwarded
  because there is no parameter to forward it to on these versions, and verl never constructs
  one, so it is always `None` in practice. A non-`None` value raises `NotImplementedError`
  rather than being silently ignored -- ignoring it would treat padded positions as real
  tokens. Idempotent per layer.
- `patch_mtp_layer_checkpointed_forward()`: log the skip-by-signature and shim counts, so
  "patch did not apply" is observable rather than inferred from missing output.

The style follows the existing precedent a few lines above in the same file, which already
probes `signature(self.mtp.forward)` for `padding_mask` before passing it.

## Test

`tests/models/test_mtp_checkpointed_forward_shim_on_cpu.py` -- 5 CPU cases, no GPU, no
distributed init, covering all three megatron-core signature generations:

- megatron-core `main` signature (declares `padding_mask`) -> shim not installed, kwarg still
  reaches the method
- 0.18.x signature -> reproduces the bare `TypeError` first, then asserts the shim accepts
  `padding_mask=None` and forwards the remaining args unchanged
- non-`None` mask -> `NotImplementedError` instead of silent corruption
- 0.14-0.17 `forward_func` signature -> left to the existing recompute patch
- shim is idempotent

Also verified end-to-end on 4x8 H20 with Qwen3.6-35B-A3B (MoE, 40 layers, 256 experts),
TP2/PP2/CP4/EP8, 64K context, `mtp_num_layers=1`, `recompute_granularity=full`,
megatron-core 0.18.2 + torch 2.11: without the shim training aborts at the first forward with
the `TypeError` above; with it, MTP trains and `mtp_losses/*` is reported normally
(30 steps, loss 0.447 -> 0.318).

Reported by / verified with AI assistance (Claude).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: gaohongkui <gaohongkui@meituan.com>
gaohongkui added a commit to gaohongkui/verl that referenced this pull request Aug 8, 2026
…tron-core 0.18.x

## What does this PR do?

Adds a compatibility shim so Multi-Token Prediction can be used together with
`recompute_granularity=full` on every released megatron-core, and makes the existing
signature-based branch in `patch_mtp_layer_checkpointed_forward` visible instead of silent.

### The bug

megatron-core 0.18.x is internally inconsistent: `MultiTokenPredictionLayer.forward` calls

    self._checkpointed_forward(..., padding_mask=padding_mask)   # multi_token_prediction.py

while `_checkpointed_forward` itself does not declare `padding_mask`. Any MTP run with
`recompute_granularity == 'full'` therefore dies at the first forward:

    TypeError: MultiTokenPredictionLayer._checkpointed_forward() got an unexpected
               keyword argument 'padding_mask'

Upstream cause: two landings that do not compose -- NVIDIA/Megatron-LM#2645 added the
call-site kwarg, verl-project#4593 refactored the method without it. Tracked as
NVIDIA/Megatron-LM#4933 (open since 2026-05-22).

It is fixed on megatron-core `main`, but `core_v0.18.2` was tagged 2026-07-20 -- two months
after that issue was filed -- and is still the newest release, so **every released
megatron-core hits this**. verl users cannot currently combine MTP with full activation
recomputation without patching megatron themselves.

### Why the existing patch does not cover it

`patch_mtp_layer_checkpointed_forward` skips any layer whose `_checkpointed_forward` does not
start with `forward_func`. That gate matches megatron-core 0.14-0.17 only; 0.18+ renamed the
first parameter to `hidden_states`, so on every 0.18+ install the patch silently does nothing:
when `target_layers` is non-empty but `patched_count == 0`, neither log line is printed, so
there is no way to tell whether it applied. Skipping is in fact *correct* on 0.18+ (megatron
now keeps non-tensor args out of the checkpoint natively, via its own `custom_forward`
closure) -- but it should say so.

## Changes

- `_patch_padding_mask_kwarg()`: when the layer's `_checkpointed_forward` lacks `padding_mask`,
  rebind it to accept and drop the kwarg. `padding_mask` is dropped rather than forwarded
  because there is no parameter to forward it to on these versions, and verl never constructs
  one, so it is always `None` in practice. A non-`None` value raises `NotImplementedError`
  rather than being silently ignored -- ignoring it would treat padded positions as real
  tokens. Idempotent per layer.
- `patch_mtp_layer_checkpointed_forward()`: log the skip-by-signature and shim counts, so
  "patch did not apply" is observable rather than inferred from missing output.

The style follows the existing precedent a few lines above in the same file, which already
probes `signature(self.mtp.forward)` for `padding_mask` before passing it.

## Test

`tests/models/test_mtp_checkpointed_forward_shim_on_cpu.py` -- 5 CPU cases, no GPU, no
distributed init, covering all three megatron-core signature generations:

- megatron-core `main` signature (declares `padding_mask`) -> shim not installed, kwarg still
  reaches the method
- 0.18.x signature -> reproduces the bare `TypeError` first, then asserts the shim accepts
  `padding_mask=None` and forwards the remaining args unchanged
- non-`None` mask -> `NotImplementedError` instead of silent corruption
- 0.14-0.17 `forward_func` signature -> left to the existing recompute patch
- shim is idempotent

Also verified end-to-end on 4x8 H20 with Qwen3.6-35B-A3B (MoE, 40 layers, 256 experts),
TP2/PP2/CP4/EP8, 64K context, `mtp_num_layers=1`, `recompute_granularity=full`,
megatron-core 0.18.2 + torch 2.11: without the shim training aborts at the first forward with
the `TypeError` above; with it, MTP trains and `mtp_losses/*` is reported normally
(30 steps, loss 0.447 -> 0.318).

Reported by / verified with AI assistance (Claude).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: gaohongkui <gaohongkui1021@163.com>
gaohongkui added a commit to gaohongkui/verl that referenced this pull request Aug 10, 2026
…tron-core 0.18.x

## What does this PR do?

Adds a compatibility shim so Multi-Token Prediction can be used together with
`recompute_granularity=full` on every released megatron-core, and makes the existing
signature-based branch in `patch_mtp_layer_checkpointed_forward` visible instead of silent.

### The bug

megatron-core 0.18.x is internally inconsistent: `MultiTokenPredictionLayer.forward` calls

    self._checkpointed_forward(..., padding_mask=padding_mask)   # multi_token_prediction.py

while `_checkpointed_forward` itself does not declare `padding_mask`. Any MTP run with
`recompute_granularity == 'full'` therefore dies at the first forward:

    TypeError: MultiTokenPredictionLayer._checkpointed_forward() got an unexpected
               keyword argument 'padding_mask'

Upstream cause: two landings that do not compose -- NVIDIA/Megatron-LM#2645 added the
call-site kwarg, verl-project#4593 refactored the method without it. Tracked as
NVIDIA/Megatron-LM#4933 (open since 2026-05-22).

It is fixed on megatron-core `main`, but `core_v0.18.2` was tagged 2026-07-20 -- two months
after that issue was filed -- and is still the newest release, so **every released
megatron-core hits this**. verl users cannot currently combine MTP with full activation
recomputation without patching megatron themselves.

### Why the existing patch does not cover it

`patch_mtp_layer_checkpointed_forward` skips any layer whose `_checkpointed_forward` does not
start with `forward_func`. That gate matches megatron-core 0.14-0.17 only; 0.18+ renamed the
first parameter to `hidden_states`, so on every 0.18+ install the patch silently does nothing:
when `target_layers` is non-empty but `patched_count == 0`, neither log line is printed, so
there is no way to tell whether it applied. Skipping is in fact *correct* on 0.18+ (megatron
now keeps non-tensor args out of the checkpoint natively, via its own `custom_forward`
closure) -- but it should say so.

## Changes

- `_patch_padding_mask_kwarg()`: when the layer's `_checkpointed_forward` lacks `padding_mask`,
  rebind it to accept and drop the kwarg. `padding_mask` is dropped rather than forwarded
  because there is no parameter to forward it to on these versions, and verl never constructs
  one, so it is always `None` in practice. A non-`None` value raises `NotImplementedError`
  rather than being silently ignored -- ignoring it would treat padded positions as real
  tokens. Idempotent per layer.
- `patch_mtp_layer_checkpointed_forward()`: log the skip-by-signature and shim counts, so
  "patch did not apply" is observable rather than inferred from missing output.

The style follows the existing precedent a few lines above in the same file, which already
probes `signature(self.mtp.forward)` for `padding_mask` before passing it.

## Test

`tests/models/test_mtp_checkpointed_forward_shim_on_cpu.py` -- 5 CPU cases, no GPU, no
distributed init, covering all three megatron-core signature generations:

- megatron-core `main` signature (declares `padding_mask`) -> shim not installed, kwarg still
  reaches the method
- 0.18.x signature -> reproduces the bare `TypeError` first, then asserts the shim accepts
  `padding_mask=None` and forwards the remaining args unchanged
- non-`None` mask -> `NotImplementedError` instead of silent corruption
- 0.14-0.17 `forward_func` signature -> left to the existing recompute patch
- shim is idempotent

Also verified end-to-end on 4x8 H20 with Qwen3.6-35B-A3B (MoE, 40 layers, 256 experts),
TP2/PP2/CP4/EP8, 64K context, `mtp_num_layers=1`, `recompute_granularity=full`,
megatron-core 0.18.2 + torch 2.11: without the shim training aborts at the first forward with
the `TypeError` above; with it, MTP trains and `mtp_losses/*` is reported normally
(30 steps, loss 0.447 -> 0.318).

Reported by / verified with AI assistance (Claude).

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: gaohongkui <gaohongkui1021@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant