Repository navigation
fix(verl): decode file URIs before loading images - #638
Jialiang Liang (lux-liang) wants to merge 1 commit into
Conversation
Convert local file URIs to native paths before opening images so their training rows are retained. Cover escaped and literal-percent filenames with real PNG files and document constructing local image URIs. Fixes microsoft#637 Generated with Codex
|
Jialiang Liang (@lux-liang) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
1 similar comment
|
Jialiang Liang (@lux-liang) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation correctly handles native paths and is covered by representative regression tests.
0 open findings
What changed in this PR
Fixes local file URI image loading in the VERL rollout adapter by converting encoded URI paths to native filesystem paths.
Changes:
- Decode file URI paths using
url2pathname. - Add regression coverage for spaces, Unicode, percent literals, and retained training rows.
- Document constructing local image URIs with
Path.as_uri().
| File | Description |
|---|---|
agentlightning/verl/rollout_adapter.py |
Converts file URIs before opening images. |
tests/verl/test_rollout_adapter.py |
Tests URI decoding and training-row retention. |
docs/80-example-multimodal-qa.md |
Documents local image URI construction. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
|
Thanks. Please sign the Contributor License Agreement then I can merge this PR. |
Fixes #637.
Images referenced with
Path.as_uri()can be dropped from training because the loader passes the escaped URI path directly to Pillow. Convert file URIs withurl2pathnamebefore opening them so local paths, spaces, Unicode, and literal percent sequences are handled correctly.The regression tests use actual PNG files and check that the public rollout adapter keeps the training row. A literal
%20filename has a distinct alternate file to detect accidental double decoding. The multimodal example now shows how to construct local image URIs.Validation:
verl-cpuenvironment:uv run --locked --no-sync pytest -v --durations=20 testspasses all 143 tests, anduv run --locked --no-sync pyrightreports no errors or warnings.uv run --locked --no-sync pytest -q tests/verl/test_rollout_adapter.py --tb=shortpasses all 22 tests.CLIPProcessor, Pillow, Torch CPU, and VERL confirms that equivalent data and file URIs now preserve the same training sample.uv run --locked --no-sync pre-commit run --all-files --show-diff-on-failure,ruff check .,ruff format --check ., andpython scripts/check_headers.pypass.uv run --locked --no-sync mkdocs build --strict,uv build --no-sources, and archive-content checks pass.Additional native Windows checks produced 134 passing tests and nine existing controller failures involving
os.killpg, plus three existingos.killpg/signal.SIGKILLtype errors. The exact original base produces the same failures and diagnostics; all controller tests and the original type check pass on Linux. WSL tests use a native Linux temporary directory because NTFS-backed temporary-file truncation fails before pytest collection.No GPU training or network-share access was tested. AI assistance: this change and its tests were prepared with Codex and independently checked by another Codex agent.