Implement stream cancelation

[jerbob92]

Fixes #332

[jerbob92]

@microsoft-github-policy-service agree company="Klippa App B.V."

[lilyydu]

Thank you for filing this!

[lilyydu]

looks like the build is failing, could you also run ruff check and ruff format in your next commit?

[jerbob92]

looks like the build is failing, could you also run ruff check and ruff format in your next commit?

Yeah I wanted to setup pre-commit, but I couldn't get it to install, had some issues with the dotnet tool nbgv.

[lilyydu]

LGTM! @heyitsaamir do you mind taking a look?

[heyitsaamir]

I like the approach here. Soemthings I think we should do:

1. Emit should be a no-op if the stream is cancelled.
2. We potentially need a way to short-circuit the AI generation if emit is short-circuiting. One approach is to throw on emit if stream is cancelled. But then we'd break all existing handlers. Another approach is for the handlers to have access to this signal, and choose to short-circuit themselves.

I'm leaning toward throwing on emit.

Thoughts?

[jerbob92]

• Emit should be a no-op if the stream is cancelled.

What do you mean? It already is.

• We potentially need a way to short-circuit the AI generation if emit is short-circuiting. One approach is to throw on emit if stream is cancelled. But then we'd break all existing handlers. Another approach is for the handlers to have access to this signal, and choose to short-circuit themselves.

I'd like the second approach more. Perhaps something using yield? (I'm not really a Python dev so not sure)

Edit:
Nvm, I see there is some EventEmitter system in place already, better to use that.

[heyitsaamir]

• Emit should be a no-op if the stream is cancelled.

What do you mean? It already is.

• We potentially need a way to short-circuit the AI generation if emit is short-circuiting. One approach is to throw on emit if stream is cancelled. But then we'd break all existing handlers. Another approach is for the handlers to have access to this signal, and choose to short-circuit themselves.

I'd like the second approach more. Perhaps something using yield? (I'm not really a Python dev so not sure)

Edit: Nvm, I see there is some EventEmitter system in place already, better to use that.

Oops you're right. Ignore 1st.

@jerbob92 why do you prefer second? I think your prompt.send throwing if the stream gets cancelled is ok.

[jerbob92]

• Emit should be a no-op if the stream is cancelled.

What do you mean? It already is.

• We potentially need a way to short-circuit the AI generation if emit is short-circuiting. One approach is to throw on emit if stream is cancelled. But then we'd break all existing handlers. Another approach is for the handlers to have access to this signal, and choose to short-circuit themselves.

I'd like the second approach more. Perhaps something using yield? (I'm not really a Python dev so not sure)
Edit: Nvm, I see there is some EventEmitter system in place already, better to use that.

Oops you're right. Ignore 1st.

@jerbob92 why do you prefer second? I think your prompt.send throwing if the stream gets cancelled is ok.

Personally I think an event based approach is cleaner, but maybe that would only really make sense if the cancelation may happen without the user doing emits, for that to work the Streamer would either need an endpoint to check for cancelation every x duration or it needs to be an actual HTTP stream. (or maybe I just didn't really get your comment and you're talking about something that's not directly related to the MR)

[lilyydu]

I like the approach here. Soemthings I think we should do:

1. Emit should be a no-op if the stream is cancelled.
2. We potentially need a way to short-circuit the AI generation if emit is short-circuiting. One approach is to throw on emit if stream is cancelled. But then we'd break all existing handlers. Another approach is for the handlers to have access to this signal, and choose to short-circuit themselves.

I'm leaning toward throwing on emit.

Thoughts?

+1 with Aamir — throwing on emit seems like the right approach. I think event-based only adds value if cancellation happens between emits

[heyitsaamir]

Yeah event emitter is less useful here because it would catch the errors asynchronously (out of band), so coordinating it back with where you're emitting in application code will be difficult.