Add streaming #1145
Add streaming #1145
Conversation
| @@ -52,6 +52,43 @@ pub fn transform( | |||
| Ok(serde_json::from_str(&results)?) | |||
| } | |||
|
|
|||
| pub fn transform_stream( | |||
There was a problem hiding this comment.
Why not replace the current transform implementation with the streaming version?
There was a problem hiding this comment.
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.
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.
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.
Why does this need to sleep?
There was a problem hiding this comment.
If I don't have it sleep I get:
ERROR: RecursionError: maximum recursion depth exceeded
There was a problem hiding this comment.
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.
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 pure else case, and sleeping for some amount of time may be a better cost/latency tradeoff.
There was a problem hiding this comment.
Is it possible to lock/wait/notify with postgres in this case?
There was a problem hiding this comment.
…formers.py to use correct TextIteratorStream
No description provided.