Skip to content

fix(verl): require full-dataset validation batches - #676

Merged
Zhiyuan He (hzy46) merged 2 commits into
microsoft:mainfrom
JiahaoTanXX:fix/require-full-validation-batch
Nov 5, 2026
Merged

Zhiyuan He (hzy46) merged 2 commits into
microsoft:mainfrom
JiahaoTanXX:fix/require-full-validation-batch

Conversation

@JiahaoTanXX

@JiahaoTanXX JiahaoTanXX (JiahaoTanXX) commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Require VERL's default data.val_batch_size=null at the start of run_ppo, with an actionable error before importing the VERL runtime helper or accessing Ray. Document the full-dataset validation requirement and correct YAML/CLI syntax.

Implements the maintainer's suggestion in #663. The dedicated assertion tests have been removed as requested in review.

Validation on macOS, Python 3.12.13, VERL 0.8.0 and PyTorch 2.13.0, using the frozen CPU dependencies:

  • .venv/bin/pytest -v --durations=20 tests: 190 passed, 4 skipped (CUDA/vLLM-dependent cases).
  • .venv/bin/pyright: 0 errors, 0 warnings.
  • .venv/bin/ruff check ., .venv/bin/ruff format --check ., .venv/bin/python scripts/check_headers.py, .venv/bin/pre-commit run --all-files --show-diff-on-failure, and git diff --check: passed.
  • .venv/bin/mkdocs build --strict: passed.
  • uv build --no-sources: source distribution and wheel built.

No live GPU training was run.

AI assistance: prepared and validated with OpenAI Codex.

Copilot AI balanced review requested due to automatic review settings November 2, 2026 07:08

Copilot AI 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.

🟢 Approval recommended

No active review issues are reported for the change enforcing full-dataset validation.

No open issues 🎉

What changed in this PR

This PR makes Agent Lightning’s full-dataset validation requirement explicit, following the maintainer recommendation in #663.

Changes:

  • Add an early assertion requiring data.val_batch_size to be None, before importing the VERL runtime helper or accessing Ray.
  • Add six regression cases covering the accepted default and rejected non-null values.
  • Document the requirement and correct YAML and CLI syntax.
File Description
tests/​verl/​test_entrypoint.py Tests valid and invalid batch-size values at the Ray startup boundary.
docs/​20-trainer-configuration.md Explains full-dataset validation and null configuration syntax.
agentlightning/​verl/​entrypoint.py Rejects non-null validation batch sizes with an actionable message.

Review effort: Balanced

@hzy46

Copy link
Copy Markdown
Contributor

Thanks. Please remove the tests. We do not need a specific test for an assert.

@hzy46
Zhiyuan He (hzy46) merged commit bc3a9e8 into microsoft:main Nov 5, 2026
6 checks passed
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.

3 participants