-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Doc] Add typing hints / mypy types cleanup #3816
Changes from 12 commits
68244df
ccc15ab
54ca154
16de54a
0724817
164b19d
e68ee26
65228f1
bd0833b
6b86585
766c23a
2f9cc7d
08eef91
bd50df3
0526dea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
from abc import ABC, abstractmethod, abstractproperty | ||
from typing import Dict, List, Optional, Protocol | ||
from abc import ABC, abstractmethod | ||
from typing import Dict, FrozenSet, List, Optional, Protocol | ||
|
||
from vllm.utils import Device | ||
|
||
|
@@ -10,23 +10,28 @@ class Block(ABC): | |
def append_token_ids(self, token_ids: List[int]) -> None: | ||
pass | ||
|
||
@abstractproperty | ||
@property | ||
@abstractmethod | ||
def block_id(self) -> Optional[int]: | ||
Comment on lines
-13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note to the audience why this change makes sense |
||
pass | ||
|
||
@abstractproperty | ||
@property | ||
@abstractmethod | ||
def token_ids(self) -> List[int]: | ||
pass | ||
|
||
@abstractproperty | ||
@property | ||
@abstractmethod | ||
def num_empty_slots(self) -> int: | ||
pass | ||
|
||
@abstractproperty | ||
@property | ||
@abstractmethod | ||
def is_full(self) -> bool: | ||
pass | ||
|
||
@abstractproperty | ||
@property | ||
@abstractmethod | ||
def prev_block(self) -> Optional["Block"]: | ||
pass | ||
|
||
|
@@ -47,12 +52,13 @@ def __call__( | |
class BlockAllocator(ABC): | ||
|
||
@abstractmethod | ||
def allocate_mutable(self, prev_block: Optional[Block]) -> Block: | ||
def allocate_mutable(self, prev_block: Optional[Block], | ||
device: Device) -> Block: | ||
pass | ||
|
||
@abstractmethod | ||
def allocate_immutable(self, prev_block: Optional[Block], | ||
token_ids: List[int]) -> Block: | ||
token_ids: List[int], device: Device) -> Block: | ||
pass | ||
|
||
@abstractmethod | ||
|
@@ -64,11 +70,12 @@ def fork(self, last_block: Block) -> List[Block]: | |
pass | ||
|
||
@abstractmethod | ||
def get_num_free_blocks(self) -> int: | ||
def get_num_free_blocks(self, device: Device) -> int: | ||
pass | ||
|
||
@abstractproperty | ||
def all_block_ids(self) -> frozenset[int]: | ||
@property | ||
@abstractmethod | ||
def all_block_ids(self) -> FrozenSet[int]: | ||
pass | ||
|
||
@abstractmethod | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import time | ||
from dataclasses import dataclass | ||
from typing import Dict, List | ||
from typing import Dict, List, Protocol | ||
|
||
import numpy as np | ||
from prometheus_client import (REGISTRY, Counter, Gauge, Histogram, Info, | ||
|
@@ -119,6 +119,12 @@ class Stats: | |
time_e2e_requests: List[float] | ||
|
||
|
||
class SupportsMetricsInfo(Protocol): | ||
|
||
def metrics_info(self) -> Dict[str, str]: | ||
... | ||
|
||
|
||
class StatLogger: | ||
"""StatLogger is used LLMEngine to log to Promethus and Stdout.""" | ||
|
||
|
@@ -135,7 +141,7 @@ def __init__(self, local_interval: float, labels: Dict[str, str]) -> None: | |
self.labels = labels | ||
self.metrics = Metrics(labelnames=list(labels.keys())) | ||
|
||
def info(self, type: str, obj: object) -> None: | ||
def info(self, type: str, obj: SupportsMetricsInfo) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. QQ: should we change the var name obj? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would slight improve readability, but I am not sure if there is any dependency on the kwarg name. |
||
if type == "cache_config": | ||
self.metrics.info_cache_config.info(obj.metrics_info()) | ||
|
||
|
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.
so unfortunate mypy cannot handle this :(
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.
On the other hand mypy hints a potential source for an actual bug - I think in this context any plausible use of that int should however also be compatible with int or float.
E.g.
int(str(0))
works butint(str(0.0))
breaks.