Commit af0f2bb
[tool] fix: avoid nested ToolResponse in SandboxFusionTool (#4833)
### What does this PR do?
Fix an incorrect double-wrapping of `ToolResponse` in
`SandboxFusionTool.execute()`.
- `execute_code()` already returns a `ToolResponse`, but `execute()`
previously wrapped it again as `ToolResponse(text=result)`.
- Since `ToolResponse.text` expects `str | None`, the old behavior could
produce an invalid/nested response (or confusing stringified output).
- This PR makes `execute()` return the `ToolResponse` directly when
appropriate, and only wraps once when the worker returns a
non-`ToolResponse` result.
### Checklist Before Starting
- [x] Search for similar PRs. Paste at least one query link here:
- [x] 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
- pre-commit:
- `pre-commit install`
- `pre-commit run --all-files --show-diff-on-failure --color=always`
- Result: **Passed**
(ruff/format/mypy/autogen-trainer-cfg/docstring/license/compileall)
### API and Usage Example
No API changes. `SandboxFusionTool.execute()` still returns
`tuple[ToolResponse, float, dict]`.
```python
# Add code snippet or script demonstrating how to use this
```
### Design & Code Changes
- `verl/tools/sandbox_fusion_tools.py`
- If the execution worker returns a `ToolResponse`, return it directly.
- Otherwise, convert the result to `str` (or `None`) and wrap once as
`ToolResponse(text=...)`.
### Checklist Before Submitting
> [!IMPORTANT]
> Please check all the following items before requesting a review,
otherwise the reviewer might deprioritize this PR for review.
- [x] Read the Contribute Guide:
https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md
- [x] Apply pre-commit checks: `pre-commit install && pre-commit run
--all-files --show-diff-on-failure --color=always`
- [ ] Add / Update the documentation:
https://github.com/volcengine/verl/tree/main/docs
- Not needed for this small bug fix.
- [ ] Add unit or end-to-end test(s) to the CI workflow:
https://github.com/volcengine/verl/tree/main/.github/workflows
- Not added. This change is a small correctness fix and is covered by
existing type/validation expectations; pre-commit checks passed.
- [ ] Once your PR is ready for CI, send a message in the `ci-request`
channel:
- https://verl-project.slack.com/archives/C091TCESWB1
- If not accessible:
https://applink.larkoffice.com/client/chat/chatter/add_by_link?link_token=772jd4f1-cd91-441e-a820-498c6614126a
- [ ] Recipe submodule update (if applicable).
- Not applicable for this PR.
Co-authored-by: winston <email@example.com>1 parent 11764c6 commit af0f2bb
1 file changed
+3
-1
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
174 | 174 | | |
175 | 175 | | |
176 | 176 | | |
177 | | - | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
178 | 180 | | |
179 | 181 | | |
180 | 182 | | |
| |||
0 commit comments