[doc, ci] chore: bump verl pin past router packaging fix (#5209)#51
Closed
SamitHuang wants to merge 1 commit intoverl-project:mainfrom
Closed
[doc, ci] chore: bump verl pin past router packaging fix (#5209)#51SamitHuang wants to merge 1 commit intoverl-project:mainfrom
SamitHuang wants to merge 1 commit intoverl-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the installation documentation by refreshing the 'Last updated' date and pinning a new git SHA for the verl library. Feedback indicates that the PR description mentions CI workflow updates that are missing from the current diff. Additionally, it is suggested to align the documentation with the default configuration in requirements.txt rather than pinning a specific git SHA to maintain consistency and avoid confusion.
| uv pip install vllm==0.18.0 | ||
| uv pip install vllm-omni==0.18 | ||
| uv pip install git+https://github.com/verl-project/verl.git@a512e90fddcefb64baaa6384e9cf8571b6bfab0b | ||
| uv pip install git+https://github.com/verl-project/verl.git@a4351480871347092436d17573ad3ccf75b24122 |
There was a problem hiding this comment.
The PR title and description mention updating the verl pin in three CI workflows, but these changes are not present in the current diff. Additionally, the documentation is updated to pin a specific git SHA while requirements.txt uses a version range (verl>=0.7.1). To avoid confusion, documentation should describe the default configuration rather than detailing specific variants like a git SHA. Please include the CI updates and ensure consistency with the default configuration in requirements.txt to prevent runtime errors.
References
- Documentation for examples should describe the default configuration to avoid confusion, rather than detailing every possible variant.
…ckaging bug The pinned verl commit (a512e90) ships a wheel that is missing verl/experimental/reward_loop/router/ because the upstream directory had no __init__.py at that commit and setuptools' default package discovery silently drops it. This breaks the FlowGRPO trainer at runtime with "ModuleNotFoundError: No module named 'verl.experimental.reward_loop.router'". Switch the verl install in docs/start/install.md from a wheel install (uv pip install git+…@<commit>) to a clone-and-editable install pinned at the same commit. An editable install exposes the source tree on sys.path, so router/ is picked up as a PEP 420 implicit namespace package and the import works without any per-venv patching. CI workflows are intentionally not touched because they don't exercise the broken codepath. The pin will be bumped past verl-project/verl#5209 once verl-omni is also adapted to the breaking LLMServerClient refactor in verl-project/verl#6129 (tracked separately).
a2fda11 to
1f9e19c
Compare
6 tasks
zhtmike
reviewed
May 1, 2026
Collaborator
zhtmike
left a comment
There was a problem hiding this comment.
This trivial bug should fix in verl side instead of doing hacking/patching?
SamitHuang
added a commit
to SamitHuang/verl-omni
that referenced
this pull request
May 1, 2026
Adapt verl-omni's diffusion agent loop and ray trainer to verl-project/verl#6129, which removed AsyncLLMServerManager and made AgentLoopManager / AgentLoopWorker consume an LLMServerClient produced by a separately-owned LLMServerManager. verl-omni changes: - DiffusionAgentLoopWorker.__init__ now takes (config, llm_client, teacher_client, reward_loop_worker_handles), matching the positional contract that AgentLoopManager.create() uses when spawning workers. _get_rollout_and_model_config was also dropped upstream, so the config slicing is inlined to keep the diff minimal. - ray_diffusion_trainer now creates an LLMServerManager first, hands its client to AgentLoopManager.create(), and uses llm_server_manager.get_replicas() (instead of async_rollout_manager.rollout_replicas) to wire the CheckpointEngineManager. This mirrors the new pattern in upstream verl/trainer/ppo/ray_trainer.py. - tests/agent_loop/test_diffusion_agent_loop.py is updated for the new API; in standalone test mode LLMServerManager spins up its own replicas via rollout.nnodes / n_gpus_per_node. Pin / docs / CI: - Bump the pinned verl commit to a4351480 (the merge commit of #5209), which is the first commit that ships verl/experimental/reward_loop/router/ in the wheel AND contains the #6129 refactor that this change adapts to. With this commit, the workaround in PR verl-project#51 (clone + editable install) is no longer required. - Restore the simple `uv pip install git+...@<commit>` install line in docs/start/install.md. - Bump the same pin in .github/workflows/{cpu_unit_tests,sanity,type-coverage-check}.yml. This is a BREAKING change because DiffusionAgentLoopWorker.__init__ signature changed. Any downstream code that subclasses or directly instantiates DiffusionAgentLoopWorker must switch from (servers, load_balancer_handle, teacher_servers, teacher_load_balancer_handle) to (llm_client, teacher_client). No public CLI/config surface is affected. Signed-off-by: samithuang <285365963@qq.com>
Collaborator
Author
|
closed as #52 will fix |
SamitHuang
added a commit
that referenced
this pull request
May 1, 2026
* [BREAKING][rollout] feat: adapt to verl LLMServerClient refactor Adapt verl-omni's diffusion agent loop and ray trainer to verl-project/verl#6129, which removed AsyncLLMServerManager and made AgentLoopManager / AgentLoopWorker consume an LLMServerClient produced by a separately-owned LLMServerManager. verl-omni changes: - DiffusionAgentLoopWorker.__init__ now takes (config, llm_client, teacher_client, reward_loop_worker_handles), matching the positional contract that AgentLoopManager.create() uses when spawning workers. _get_rollout_and_model_config was also dropped upstream, so the config slicing is inlined to keep the diff minimal. - ray_diffusion_trainer now creates an LLMServerManager first, hands its client to AgentLoopManager.create(), and uses llm_server_manager.get_replicas() (instead of async_rollout_manager.rollout_replicas) to wire the CheckpointEngineManager. This mirrors the new pattern in upstream verl/trainer/ppo/ray_trainer.py. - tests/agent_loop/test_diffusion_agent_loop.py is updated for the new API; in standalone test mode LLMServerManager spins up its own replicas via rollout.nnodes / n_gpus_per_node. Pin / docs / CI: - Bump the pinned verl commit to a4351480 (the merge commit of #5209), which is the first commit that ships verl/experimental/reward_loop/router/ in the wheel AND contains the #6129 refactor that this change adapts to. With this commit, the workaround in PR #51 (clone + editable install) is no longer required. - Restore the simple `uv pip install git+...@<commit>` install line in docs/start/install.md. - Bump the same pin in .github/workflows/{cpu_unit_tests,sanity,type-coverage-check}.yml. This is a BREAKING change because DiffusionAgentLoopWorker.__init__ signature changed. Any downstream code that subclasses or directly instantiates DiffusionAgentLoopWorker must switch from (servers, load_balancer_handle, teacher_servers, teacher_load_balancer_handle) to (llm_client, teacher_client). No public CLI/config surface is affected. Signed-off-by: samithuang <285365963@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bump the pinned verl SHA from a512e90 to a4351480 (the merge commit of verl-project/verl#5209) in docs/start/install.md and the three CI workflows that pin verl explicitly. The previous pin shipped a wheel without verl/experimental/reward_loop/router/ because the upstream directory had no init.py and setuptools' default package discovery silently dropped it, breaking the FlowGRPO trainer at runtime with "ModuleNotFoundError: No module named 'verl.experimental.reward_loop.router'".
The merge commit adds the missing init.py, so the wheel now includes the directory and the FlowGRPO trainer imports cleanly.
What does this PR do?
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,vllm_omni,rollout,trainer,ci,training_utils,recipe,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,diffusion,omni,tests,docker,like[diffusion, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][diffusion, fsdp] feat: new rollout schedulerTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always