-
Notifications
You must be signed in to change notification settings - Fork 181
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
Productionize Chat demo #1235
Productionize Chat demo #1235
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.
- test with different models (llama, mpt)
- add prompt_length to the arguments
- add a docstring showing a chat example
- streaming mode (optional scenario)
- display tokens per second; optional (default to False) --> timer manager
Noting will add streaming mode as a part of another PR all other comments have been addressed! |
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.
Can we have a short demo output in the PR description?
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.
LGTM overall; two comments:
- nit: name
main.py
is super generic - a docstring of an expected output might be helpful
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.
Is chatbot.py
supposed to replace main.py
- looks like they are duplicated. Also I assume using the session_id
keeps the previous history to feed in each time? Can there be an option to not keep history and instead just feed in the current prompt?
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 don't think an __init__.py
is needed for an example and it would be nice to have a small README.md - nice changes!
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.
Nice!
cceeddf
to
17a2aa4
Compare
The base branch was changed.
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.
LGTM for initial land - let's get a persistent session id added in
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.
let's rename this to and infer.py and move it to source. also see comment on history
Add persistent session_id Delete init
17a2aa4
to
09eec2d
Compare
Update fire condition
Add cli callable
This PR is a productionized version of #1229 originally written by @dbogunowicz;
this is essentially the same code with:
Note: streaming mode will be added as a part of a separate PR
Example usage:
Output: