[BugFix] Fix cache issue in compilation_config#31376
[BugFix] Fix cache issue in compilation_config#31376yewentao256 merged 3 commits intovllm-project:mainfrom
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. |
There was a problem hiding this comment.
Code Review
This pull request addresses a caching bug in get_cached_compilation_config by ensuring the cache is cleared upon entering the set_current_vllm_config context manager. This prevents an old, cached configuration from being used when a new configuration is set. The fix is correct and directly solves the issue described. The added code comment clearly explains the necessity of this change. The implementation is sound, and I have no further recommendations.
yewentao256
left a comment
There was a problem hiding this comment.
LGTM, thanks for the work!
Is there any specific issue this PR could solve? (If there is a case that without this PR would cause trouble?)
Signed-off-by: Boyuan Feng <boyuan@meta.com>
|
@yewentao256 I added a unit test |
yewentao256
left a comment
There was a problem hiding this comment.
LGTM, thanks for the work!
yewentao256
left a comment
There was a problem hiding this comment.
Merge this PR as all CI tests pass
Signed-off-by: Boyuan Feng <boyuan@meta.com>
Signed-off-by: Boyuan Feng <boyuan@meta.com>
Signed-off-by: Boyuan Feng <boyuan@meta.com>
#22204 added
get_cached_compilation_config()with@lru_cache(maxsize=1)to cache compilation config access viaSince
get_cached_compilation_config()has no argument, it would never be refreshed, unless we explicitly callget_cached_compilation_config.cache_clear().To refresh the config, #22204 implements
get_cached_compilation_config.cache_clear()inset_current_vllm_config.However, a bug would happen if an
old_vllm_confighas been accessed and cached beforeset_current_vllm_config(new_config)is called. In other wordsThis PR fixes the issue by clearing the cache when entering the context manager.