[Platform] Deprecate seed_everything#31659
Conversation
|
Documentation preview: https://vllm--31659.org.readthedocs.build/en/31659/ |
3016790 to
6a73893
Compare
There was a problem hiding this comment.
Code Review
This pull request deprecates the seed_everything method from the platform interface and centralizes random seed setting into a new set_random_seed utility function in vllm.utils.torch_utils. This is a good cleanup that simplifies the platform interface. The changes involve updating numerous test files and internal utilities to use the new function.
I've identified a critical issue where the refactoring of set_random_seed is incomplete, which will break the build. I've also found several instances of redundant seed setting in the test files after the changes. Please see my comments for details on how to address these issues.
I am having trouble creating individual review comments. Click here to see my feedback.
vllm/model_executor/utils.py (13-16)
Moving set_random_seed to vllm.utils.torch_utils is a good refactoring. However, this change is incomplete as it doesn't update the call sites of this function. This will cause import errors and break the build.
The following files need to be updated to import set_random_seed from vllm.utils.torch_utils instead of vllm.model_executor.utils:
vllm/engine/llm_engine.pyvllm/worker/worker_base.py
Please update these files to complete the refactoring.
tests/kernels/attention/test_lightning_attn.py (125-126)
The call to set_random_seed(42) on line 127 already sets the seed for torch on all devices. These calls to torch.manual_seed and torch.cuda.manual_seed_all are redundant and can be removed.
tests/kernels/attention/test_lightning_attn.py (168-169)
The call to set_random_seed(42) on line 170 already sets the seed for torch on all devices. These calls to torch.manual_seed and torch.cuda.manual_seed_all are redundant and can be removed.
tests/kernels/attention/test_lightning_attn.py (232-233)
The call to set_random_seed(42) on line 234 already sets the seed for torch on all devices. These calls to torch.manual_seed and torch.cuda.manual_seed_all are redundant and can be removed.
tests/kernels/quantization/test_mxfp4_qutlass.py (210-211)
The call to set_random_seed(0) on line 209 already sets the seed for numpy and torch. These subsequent calls are redundant and can be removed.
tests/kernels/quantization/test_nvfp4_qutlass.py (198-199)
The call to set_random_seed(0) on line 197 already sets the seed for numpy and torch. These subsequent calls are redundant and can be removed.
|
Doesn't |
DarkLight1337
left a comment
There was a problem hiding this comment.
Edit: Contrary to the PR description, set_random_seed handles the other modules as well so it is fine
|
@DarkLight1337 My fault. Updated the commit message. |
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
Head branch was pushed to by a user without write access
6a73893 to
728a8fc
Compare
yewentao256
left a comment
There was a problem hiding this comment.
LGTM, thanks for the work!
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
Purpose
seed_everythingplatform interface is totally useless, no platform redefine it It's always the same withLet's deprecate this interface to make the platform interface more clean.
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.