[0.13.0][Profiler] Fix profiler bug#6383
[0.13.0][Profiler] Fix profiler bug#6383wangxiyuan merged 1 commit intovllm-project:releases/v0.13.0from
Conversation
wangxiyuan
changed the title
![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 appears to fix a bug in the profiler by changing some of its configuration parameters. Specifically, it enables data_simplification, disables with_stack, and repurposes the VLLM_TORCH_PROFILER_WITH_STACK environment variable to control with_modules. While disabling with_stack is likely the intended bug fix, repurposing an environment variable with a now-misleading name is a concern for maintainability and user experience. I've left a comment with a suggestion to improve this.
| with_stack=False, | ||
| profile_memory=envs_vllm.\ | ||
| VLLM_TORCH_PROFILER_WITH_PROFILE_MEMORY, | ||
| with_modules=False, | ||
| with_modules=bool(envs_vllm.VLLM_TORCH_PROFILER_WITH_STACK), |
There was a problem hiding this comment.
While hardcoding with_stack=False seems to address a bug, repurposing the VLLM_TORCH_PROFILER_WITH_STACK environment variable to control with_modules is confusing for users and harms maintainability. The variable name no longer matches its behavior, which is a breaking change in functionality for users of this flag.
To improve clarity, it would be better to introduce a new, descriptively named environment variable (e.g., VLLM_TORCH_PROFILER_WITH_MODULES) for this purpose. If that's not feasible, at least a comment explaining this change is crucial to avoid future confusion.
linfeng-yuan
left a comment
linfeng-yuan
left a comment
There was a problem hiding this comment.
- Maybe
envs_vllm.VLLM_TORCH_PROFILER_WITH_STACKis a string; - change profile_memory=envs_vllm.
VLLM_TORCH_PROFILER_WITH_PROFILE_MEMORY, also
wangxiyuan
force-pushed
the
fix_profiler
branch
from
cd61223 to
1b96018
Compare
Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
wangxiyuan
force-pushed
the
fix_profiler
branch
from
1b96018 to
2b9ff41
Compare
Pick from vllm-project#6141 to make profiler work as expect. This PR also make sure `VLLM_TORCH_PROFILER_WITH_STACK` and `VLLM_TORCH_PROFILER_WITH_PROFILE_MEMORY` work as the same behaviour with v0.12.0 Signed-off-by: wangxiyuan <wangxiyuan1007@gmail.com>
2 participants
Pick from #6141 to make profiler work as expect.
This PR also make sure
VLLM_TORCH_PROFILER_WITH_STACKandVLLM_TORCH_PROFILER_WITH_PROFILE_MEMORYwork as the same behaviour with v0.12.0