- Notifications
You must be signed in to change notification settings - Fork328
Add streaming#1145
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add streaming#1145
Uh oh!
There was an error while loading.Please reload this page.
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.
Very exciting!
| @@ -52,6 +52,43 @@ pub fn transform( | |||
| Ok(serde_json::from_str(&results)?) | |||
| } | |||
| pub fn transform_stream( | |||
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.
Why not replace the current transform implementation with the streaming version?
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 guess if the user calls it without a limit or cursor, they do the same exact thing. I'm not sure what the overhead of dipping into python for every token is. If that isn't an issue I totally can!
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 love the focus on performance. I think we can keep the two different implementations under a single API function, with a fork in the logic depending on the params. i.e. the caller shouldn't have to think about that as a high level split, and we shouldn't have to duplicate all the documentation for the function, just the change in behavior based on those arguments.
| let args = serde_json::to_string(args)?; | ||
| let inputs = serde_json::to_string(&inputs)?; | ||
| Python::with_gil(|py| -> Result<Py<PyAny>> { |
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 probably move all this into the transformers module just for organization.
| return self.done_data.pop(0) | ||
| elif self.done: | ||
| raise StopIteration | ||
| time.sleep(0.1) |
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.
Why does this need to sleep?
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.
If I don't have it sleep I get:ERROR: RecursionError: maximum recursion depth exceeded
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.
That's weird...
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.
This is because we hit the recursion depth before a new token is added to done_data. I'd suggest moving to a while loop rather than recursing since Python is a ... does not support tail recursion.
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.
After moving to a while loop, I think it's worth a big fat comment warning that we are spin locking in this thread, and sleeping for some amount of time on each iteration in a pureelse case, and sleeping for some amount of time may be a better cost/latency tradeoff.
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 it possible to lock/wait/notify with postgres in this case?
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 description provided.