-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add StableLM3B model #2372
Add StableLM3B model #2372
Conversation
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.
Hi @ita9naiwa, thanks for submitting the PR! It looks good overall, but needs some minor stylistic changes. Please check out my comments.
KVCache = Tuple[torch.Tensor, torch.Tensor] | ||
|
||
|
||
class StableLMEpochConfig(PretrainedConfig): |
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.
Do we need this class?
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.
No, I can remove this class and pass arguments directly.
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.
models like llama in vllm (models/llama.py) there are config classes.
# (llama.py)
from transformers import LlamaConfig
# (yi.py)
from vllm.transformers_utils.configs.yi import YiConfig
StableLM does not have official transformers implementations so I guess it's better to have StableLMEpochConfig to carry configs.
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.
because passing all arguments for the model will make code really verbose
I'm just asking and I'll follow your opinion
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.
I think the discussion point here is that StableLMEpochConfig
is never initialized. When vLLM is reading the config, it is read into PretrainedConfig, which you can also access the model specific attributes using config.vocab_size
. This class currently only serve as documentation purpose.
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.
@simon-mo Thank you notifying that. I'll remove StableLMEpochConfig
class.
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.
I removed StableLMEpochConfig
class.
Co-authored-by: Woosuk Kwon <[email protected]>
I checked using |
confirming examples
|
Resolves #1426
Add new model StableLM