Skip to content

Conversation

zhanghw0354
Copy link
Contributor

@zhanghw0354 zhanghw0354 commented Aug 28, 2025

What this PR does / why we need it?

Currently, when performing profiling in vLLM-Ascend, if you need to obtain the Python call stack, you have to manually modify the code. The code location is: worker_v1.py#L337 where you set with_stack to true.
Now, in vLLM, you can set whether to obtain the Python call stack through an environment variable. The relevant PR is: #21803 and the documentation is: profiling
This PR sets the profiler initialization parameters by using the same environment variable as vLLM, eliminating the need for manual code modification.

Does this PR introduce any user-facing change?

No

How was this patch tested?

CI passed with new added/existing test.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aligns the profiler configuration in vllm-ascend with the upstream vllm project by allowing with_stack and profile_memory parameters to be set via environment variables. The changes are correct and the tests have been updated accordingly. I've added one high-severity suggestion to use getattr for accessing these new environment variables. This will make the implementation more robust and backward-compatible with older versions of vllm that may not have these variables defined, preventing potential AttributeError exceptions at runtime.

Comment on lines 337 to 338
with_stack=envs_vllm.VLLM_TORCH_PROFILER_WITH_STACK,
profile_memory=envs_vllm.VLLM_TORCH_PROFILER_WITH_PROFILE_MEMORY,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

To improve robustness and ensure backward compatibility, it's better to use getattr with a default value when accessing these environment variables from envs_vllm. This will prevent an AttributeError if vllm-ascend is run with an older version of vllm where VLLM_TORCH_PROFILER_WITH_STACK or VLLM_TORCH_PROFILER_WITH_PROFILE_MEMORY are not defined. The default value of False will maintain the previous behavior in such cases.

It would also be beneficial to add a new unit test to verify this fallback behavior, ensuring the profiler initializes with with_stack=False and profile_memory=False when the environment variables are not present.

Suggested change
with_stack=envs_vllm.VLLM_TORCH_PROFILER_WITH_STACK,
profile_memory=envs_vllm.VLLM_TORCH_PROFILER_WITH_PROFILE_MEMORY,
with_stack=getattr(envs_vllm, "VLLM_TORCH_PROFILER_WITH_STACK", False),
profile_memory=getattr(envs_vllm, "VLLM_TORCH_PROFILER_WITH_PROFILE_MEMORY", False),

Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.50%. Comparing base (fef18b6) to head (79df9e2).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
vllm_ascend/worker/worker_v1.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2608   +/-   ##
=======================================
  Coverage   73.49%   73.50%           
=======================================
  Files         151      151           
  Lines       21927    21929    +2     
=======================================
+ Hits        16116    16118    +2     
  Misses       5811     5811           
Flag Coverage Δ
unittests 73.50% <80.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zhanghw0354 zhanghw0354 force-pushed the add-profiler-env branch 2 times, most recently from 5b96065 to 782e846 Compare September 1, 2025 07:43
zhanghaiwen added 5 commits September 2, 2025 10:20
Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
@zhanghw0354
Copy link
Contributor Author

@wangxiyuan @Yikun Please review when you have time. Thank you!

@wangxiyuan wangxiyuan merged commit eaeb2ef into vllm-project:main Sep 3, 2025
28 checks passed
@zhanghw0354 zhanghw0354 deleted the add-profiler-env branch September 3, 2025 03:07
offline893 pushed a commit to offline893/vllm-ascend that referenced this pull request Sep 16, 2025
… consistent with vLLM (vllm-project#2608)

### What this PR does / why we need it?
Currently, when performing profiling in vLLM-Ascend, if you need to
obtain the Python call stack, you have to manually modify the code. The
code location is:
[worker_v1.py#L337](https://github.com/vllm-project/vllm-ascend/blob/6c973361fc2eba5d3faa9b6b496b4b9fec4dc784/vllm_ascend/worker/worker_v1.py#L337)
where you set with_stack to true.
Now, in vLLM, you can set whether to obtain the Python call stack
through an environment variable. The relevant PR is:
[#21803](vllm-project/vllm#21803) and the
documentation is:
[profiling](https://docs.vllm.ai/en/latest/contributing/profiling.html?h=vllm_torch_profiler_with_stack#profile-with-pytorch-profiler)
This PR sets the profiler initialization parameters by using the same
environment variable as vLLM, eliminating the need for manual code
modification.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
CI passed with new added/existing test.

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@0235103

---------

Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Co-authored-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Signed-off-by: offline0806 <z00858301@china.huawei.com>
wangxiaoteng888 pushed a commit to LCAIZJ/vllm-ascend that referenced this pull request Sep 25, 2025
… consistent with vLLM (vllm-project#2608)

### What this PR does / why we need it?
Currently, when performing profiling in vLLM-Ascend, if you need to
obtain the Python call stack, you have to manually modify the code. The
code location is:
[worker_v1.py#L337](https://github.com/vllm-project/vllm-ascend/blob/6c973361fc2eba5d3faa9b6b496b4b9fec4dc784/vllm_ascend/worker/worker_v1.py#L337)
where you set with_stack to true.
Now, in vLLM, you can set whether to obtain the Python call stack
through an environment variable. The relevant PR is:
[#21803](vllm-project/vllm#21803) and the
documentation is:
[profiling](https://docs.vllm.ai/en/latest/contributing/profiling.html?h=vllm_torch_profiler_with_stack#profile-with-pytorch-profiler)
This PR sets the profiler initialization parameters by using the same
environment variable as vLLM, eliminating the need for manual code
modification.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
CI passed with new added/existing test.

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@0235103

---------

Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Co-authored-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
chopper0126 pushed a commit to chopper0126/vllm-ascend that referenced this pull request Sep 26, 2025
… consistent with vLLM (vllm-project#2608)

### What this PR does / why we need it?
Currently, when performing profiling in vLLM-Ascend, if you need to
obtain the Python call stack, you have to manually modify the code. The
code location is:
[worker_v1.py#L337](https://github.com/vllm-project/vllm-ascend/blob/6c973361fc2eba5d3faa9b6b496b4b9fec4dc784/vllm_ascend/worker/worker_v1.py#L337)
where you set with_stack to true.
Now, in vLLM, you can set whether to obtain the Python call stack
through an environment variable. The relevant PR is:
[#21803](vllm-project/vllm#21803) and the
documentation is:
[profiling](https://docs.vllm.ai/en/latest/contributing/profiling.html?h=vllm_torch_profiler_with_stack#profile-with-pytorch-profiler)
This PR sets the profiler initialization parameters by using the same
environment variable as vLLM, eliminating the need for manual code
modification.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
CI passed with new added/existing test.

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@0235103

---------

Signed-off-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Co-authored-by: zhanghaiwen <zhanghaiwen@cmss.chinamobile.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants