[Bugfix] tpu_model_runner: set vllm config context when calling reset_dynamo_cache()#30331
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
b9b0940 to
d73490b
Compare
There was a problem hiding this comment.
Code Review
This pull request addresses a startup crash in the tpu_model_runner by correctly setting the VllmConfig context. The issue stemmed from an attempt to access the configuration too early during the re-initialization of TorchCompileWithNoGuardsWrapper. The fix, which involves wrapping the __init__ call with the set_current_vllm_config context manager, is a direct and effective solution to the problem. The change is well-targeted and correctly applies the established pattern for managing configuration context within the codebase. I find the fix to be correct and necessary.
|
Hi @dtrifiro, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
d73490b to
eb59cfa
Compare
NickLucche
left a comment
There was a problem hiding this comment.
Thanks for the fix @dtrifiro , this makes sense to me!
One suggestion: what about moving the set_current_vllm_config call to tpu_worker.py, so that it better matches the use of it we're doing in gpu_worker.py?
eb59cfa to
d04ff3a
Compare
…ache() switching over to `TorchCompileWithNoGuardsWrapper.__init__(compiled_model)` causes a startup crash due to an attempt to access `VllmConfig` too early. See https://github.com/vllm-project/vllm/pull/25110/files#r2593461144 Signed-off-by: Daniele Trifirò <dtrifiro@redhat.com>
d04ff3a to
55d6819
Compare
|
Thanks @NickLucche, excellent point. Updated as suggested and verified that it actually works. |
|
Test failures look unrelated, can this be merged? Failures:
|
…_dynamo_cache() (vllm-project#30331) Signed-off-by: Daniele Trifirò <dtrifiro@redhat.com> Signed-off-by: Ubuntu <mjtaheri68@gmail.com>
…_dynamo_cache() (vllm-project#30331) Signed-off-by: Daniele Trifirò <dtrifiro@redhat.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
The switch to
TorchCompileWithNoGuardsWrapper.__init__(compiled_model)in #25110 causes a startup crashdue to an attempt to access
VllmConfigtoo early.See https://github.com/vllm-project/vllm/pull/25110/files#r2593461144