-
Notifications
You must be signed in to change notification settings - Fork 491
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
inference: Add model option to client #170
inference: Add model option to client #170
Conversation
d58b2b6
to
1499006
Compare
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.
lg
llama_stack/apis/inference/client.py
Outdated
messages=[message], | ||
stream=stream, | ||
) | ||
async for log in EventLogger().log(iterator): | ||
log.print() | ||
|
||
|
||
async def run_mm_main(host: str, port: int, stream: bool, path: str): | ||
async def run_mm_main(host: str, port: int, stream: bool, path: str, model: str): |
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.
nit: (also existing issue) -- the typehint for both path
and model
should be Optional[str]
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.
yeah, I noticed a warning because of that. I can go ahead and fix it here.
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.
@ashwinb added a fix for that to this PR
llama_stack/apis/inference/client.py
Outdated
messages=[message], | ||
stream=stream, | ||
) | ||
async for log in EventLogger().log(iterator): | ||
log.print() | ||
|
||
|
||
def main(host: str, port: int, stream: bool = True, mm: bool = False, file: str = None): | ||
def main(host: str, port: int, stream: bool = True, mm: bool = False, file: Optional[str] = None, model: Optional[str] = None): |
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.
could you run pre-commit
? make sure you install it first
cd llama-stack
pip install pre-commit
pre-commit install
Then run git commit -a --amend
(once this time) so it will run and run all the formatting lints.
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.
Oops, yes, it's about time I install that!
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.
done
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.
... and now with automation so you don't have to catch this yourself! #176
da51106
to
e91e9cc
Compare
I was running this client for testing purposes and being able to specify which model to use is a convenient addition. This change makes that possible. Signed-off-by: Russell Bryant <rbryant@redhat.com>
A couple of arguments were Optional string arguments, but not marked as such. We were getting some warnings as a result. Signed-off-by: Russell Bryant <rbryant@redhat.com>
e91e9cc
to
242632a
Compare
I was running this client for testing purposes and being able to
specify which model to use is a convenient addition. This change makes
that possible.
Signed-off-by: Russell Bryant rbryant@redhat.com