-
Notifications
You must be signed in to change notification settings - Fork 9
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
Streaming support #96
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.
Nice work! Left some comments.
Also could you please add an example either in a doctest (in load.py) or an example in examples/logging
?
logger.error(f"LOG10: failed to insert in log10: {self.partial_log_row} with error {res.text}") | ||
except Exception as e: | ||
traceback.print_tb(e.__traceback__) | ||
logging.warn(f"LOG10: failed to log: {e}. Skipping") |
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.
logging.warn(f"LOG10: failed to log: {e}. Skipping") | |
logger.error(f"LOG10: failed to log: {e}. Skipping") |
be consistent.
Also thinking we could consider to raise the logging level even higher to logger.critical
for LOG10 failures. https://docs.python.org/3/library/logging.html#logging.Logger.critical
in a follow up PR.
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.
Think we are using logging.warn
other places?
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.
yea, seems there's a mix of logger.error and logger.warn in the load.py. could do a clean up in a follow up PR.
traceback.print_tb(e.__traceback__) | ||
logging.warn(f"LOG10: failed to log: {e}. Skipping") | ||
|
||
raise se |
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.
just to verify, this is the StopIterator
so we want to raise it to tell user the stream has finished.
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.
Yep that's right
log10/load.py
Outdated
@@ -359,6 +422,26 @@ def wrapper(*args, **kwargs): | |||
|
|||
response = Anthropic.prepare_response(kwargs["prompt"], output, "text") | |||
kind = "completion" | |||
elif type(output).__name__ == "Stream": | |||
response = output |
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.
duplicated on ln 418. remove?
log10/load.py
Outdated
"choices": [ | ||
{ | ||
"index": 0, | ||
"finish_reason": "stop", |
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 max_token reached be the other reason?
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 see if we can get the stop reason from the last chunk above. Will give that a go
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.
Worked - should have been updated
log10/load.py
Outdated
def get_final_result(self): | ||
return self.final_result |
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.
curious is this been called somewhere now or just a util function? Do we need to check StopIterator or has been raised or some state telling that this is the final results?
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.
We can remove it
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. please add the example before merging.
No description provided.