Revert "[rollout] feat: use dummy load_format when init AsyncServer"#3207
Revert "[rollout] feat: use dummy load_format when init AsyncServer"#3207vermouth1992 merged 1 commit intomainfrom
Conversation
vermouth1992
requested review from
PeterSH6,
chenhaiq and
wuxibin89
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reverts a change that attempted to use a configurable load_format for the AsyncvLLMServer. The revert is likely intended to fix a crash caused by passing an invalid value to vLLM.
However, simply reverting to a hardcoded "auto" introduces an inconsistency between the synchronous and asynchronous vLLM rollout implementations. The synchronous mode correctly handles dummy load_format values from the configuration, which is crucial for scenarios like loading weights from FSDP checkpoints. The asynchronous mode, with this revert, will silently ignore this configuration and always attempt to load weights from the model path.
I've added a comment with a suggestion to fix this inconsistency by correctly handling the load_format configuration in the asynchronous server, similar to how it's done in the synchronous implementation. This will ensure both modes behave consistently and support all configured features.
| max_model_len=self.max_model_len, | ||
| max_num_seqs=config.max_num_seqs, | ||
| load_format=config.load_format, | ||
| load_format="auto", |
There was a problem hiding this comment.
This change reverts the load_format to a hardcoded "auto". While this fixes the issue from the original commit where an invalid value might be passed, it creates an inconsistency with the synchronous vLLMRollout implementation and disables a potentially important feature for AsyncvLLMServer.
The synchronous counterpart in verl/workers/rollout/vllm_rollout/vllm_rollout_spmd.py handles dummy load formats correctly. By reverting to "auto", the async server will ignore the load_format from the configuration, which can lead to unexpected behavior, especially when external weight loading is required.
To maintain consistency and support for dummy loading in async mode, consider applying the same logic as in the sync implementation.
| load_format="auto", | |
| load_format="dummy" if config.load_format.startswith("dummy") else config.load_format, |
|
@vermouth1992 Hi, may I ask the reason behind this revert? I got an error during the training and this argument is one of my suspects. |
3 participants
Reverts #3184