- 
                Notifications
    
You must be signed in to change notification settings  - Fork 533
 
[Bug] Fix bug in test_chunked.py #1992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
          Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@           Coverage Diff           @@
##             main    #1992   +/-   ##
=======================================
  Coverage   76.09%   76.09%           
=======================================
  Files         114      114           
  Lines       13103    13103           
=======================================
  Hits         9971     9971           
  Misses       3132     3132           
 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
  | 
    
f7b9364    to
    b2af312      
    Compare
  
    | 
           cc @Yikun @wangxiyuan take a review when free, thanks  | 
    
        
          
                tests/e2e/singlecard/test_chunked.py
              
                Outdated
          
        
      | vllm_model = LLM(model, long_prefill_token_threshold=4, enforce_eager=True) | ||
| output_chunked = vllm_model.generate(prompts, sampling_params) | ||
| logprobs_chunked = output_chunked.outputs[0].logprobs | ||
| vllm_model = LLM(model, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change, can you use VllmRunner instead?
vllm-ascend/tests/e2e/conftest.py
Line 81 in 2da281e
| class VllmRunner: | 
It can cleanup resouce by __exit__ func auomaticlly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change, can you use VllmRunner instead?
vllm-ascend/tests/e2e/conftest.py
Line 81 in 2da281e
class VllmRunner: It can cleanup resouce by
__exit__func auomaticlly.
Nice!!!
7bfafb4    to
    2e302c4      
    Compare
  
    fb5618d    to
    ddd761c      
    Compare
  
    Signed-off-by: xleoken <xleoken@163.com>
| 
           cc @wangxiyuan @Yikun take a look when free, the ci has passed.  | 
    
### What this PR does / why we need it? 1. Remove the return statement, it will always skip following logic. 2. Update `deepseek` to `Qwen2.5-Instruct` for OOM in github e2e test env. 3. Fix the comparison logic ### Does this PR introduce _any_ user-facing change? NO. ### How was this patch tested? Local Test. - vLLM version: v0.10.0 - vLLM main: vllm-project/vllm@0933f9d Signed-off-by: xleoken <xleoken@163.com>
### What this PR does / why we need it? 1. Remove the return statement, it will always skip following logic. 2. Update `deepseek` to `Qwen2.5-Instruct` for OOM in github e2e test env. 3. Fix the comparison logic ### Does this PR introduce _any_ user-facing change? NO. ### How was this patch tested? Local Test. - vLLM version: v0.10.0 - vLLM main: vllm-project/vllm@0933f9d Signed-off-by: xleoken <xleoken@163.com>
### What this PR does / why we need it? 1. Remove the return statement, it will always skip following logic. 2. Update `deepseek` to `Qwen2.5-Instruct` for OOM in github e2e test env. 3. Fix the comparison logic ### Does this PR introduce _any_ user-facing change? NO. ### How was this patch tested? Local Test. - vLLM version: v0.10.0 - vLLM main: vllm-project/vllm@0933f9d Signed-off-by: xleoken <xleoken@163.com>
What this PR does / why we need it?
Remove the return statement, it will always skip following logic.
Update
deepseektoQwen2.5-Instructfor OOM in github e2e test env.Fix the comparison logic
Does this PR introduce any user-facing change?
NO.
How was this patch tested?
Local Test.