[Chore] fix spelling in kv_transfer_config variable name#26330
[Chore] fix spelling in kv_transfer_config variable name#26330natoscott wants to merge 1 commit intovllm-project:mainfrom
Conversation
Signed-off-by: Nathan Scott <nathans@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a spelling mistake in the kv_transfer_config variable name across two files, improving code consistency. While reviewing, I identified a potential issue in KVConnectorLogging.__init__ where an instance attribute might not be initialized in all code paths. I've provided a suggestion to make the code more robust against potential AttributeError exceptions.
| def __init__(self, kv_transfer_config: KVTransferConfig): | ||
| # This should be called on frontend process. | ||
| assert not has_kv_transfer_group() | ||
| # Instantiate the connector's stats class. | ||
| if kv_tranfer_config and kv_tranfer_config.kv_connector: | ||
| if kv_transfer_config and kv_transfer_config.kv_connector: | ||
| self.connector_cls = KVConnectorFactory.get_connector_class( | ||
| kv_tranfer_config | ||
| kv_transfer_config | ||
| ) | ||
| self.reset() |
There was a problem hiding this comment.
The attribute self.connector_cls is only initialized within the if block. If the condition is false, self.connector_cls is never set. While the current call site for observe() seems to be safe, this can lead to an AttributeError in the future if observe() is called. It's best practice to initialize all instance attributes in __init__. Consider initializing self.connector_cls to None before the conditional block to make the class more robust.
| def __init__(self, kv_transfer_config: KVTransferConfig): | |
| # This should be called on frontend process. | |
| assert not has_kv_transfer_group() | |
| # Instantiate the connector's stats class. | |
| if kv_tranfer_config and kv_tranfer_config.kv_connector: | |
| if kv_transfer_config and kv_transfer_config.kv_connector: | |
| self.connector_cls = KVConnectorFactory.get_connector_class( | |
| kv_tranfer_config | |
| kv_transfer_config | |
| ) | |
| self.reset() | |
| def __init__(self, kv_transfer_config: KVTransferConfig): | |
| # This should be called on frontend process. | |
| assert not has_kv_transfer_group() | |
| # Instantiate the connector's stats class. | |
| self.connector_cls = None | |
| if kv_transfer_config and kv_transfer_config.kv_connector: | |
| self.connector_cls = KVConnectorFactory.get_connector_class( | |
| kv_transfer_config | |
| ) | |
| self.reset() |
There was a problem hiding this comment.
Not something introduced by this PR, and while not ideal, we would hit the AttributeError here:
assert self.connector_cls is not None
which is fine
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
Fixed by #29674, thanks! |
Purpose
Fixes an spelling error on a variable naming (missing 's' in kv_transfer_config).
Test Plan
Ran a 'vllm bench throughput' test to sanity check.
Test Result
All is well.