feat: support stream api

[ganisback]

currently jupyter-server-proxy does not support the text/event-stream api. when we install https://github.com/hiyouga/LLaMA-Factory in the notebook instance, and access by : http://xxx.xxx/proxy/7860/ the normal request works fine, but the stream API never response. reference code: https://github.com/ideonate/jhsingle-native-proxy/blob/3e26a4ee0e7318970b7cf6abbd7d88455a9ac621/jhsingle_native_proxy/proxyhandlers.py#L217

[ganisback]

@yuvipanda please help review this feature

[manics]

@ganisback Thanks for your contribution! To help with review please could you expand the PR description to explain what this does, and why it's needed? We'll need some tests if this is going to be merged, but those can be added later if we agree this feature should be added.

[ganisback]

@manics description updated

[yuvipanda]

Ah interesting! I think @jmunroe also ran into this recently.

If my understanding is right, the existing code doesn't stream but waits for the entire upstream request to be completed before passing it on. And with this PR, we're special casing the text/eventstream to be streaming.

Instead, what do you think about modifying the existing code (what you have as _proxy_normal) to be streaming? I don't think there's any reason to not stream all requests - I think we initially waited for requests to complete simply because that was the easier thing to do. That would also help with tests - the current tests should catch that and if they pass, we can probably merge that.

So I propose that instead of only streaming eventstreams, we stream everything.

Can't do this, see https://github.com/jupyterhub/jupyter-server-proxy/pull/479#issuecomment-2177338362

[yuvipanda]

Ah damn, I just realized that won't work, because of our RewriteableResponse implementation :( That implementation's signature depends on being able to buffer requests, rather than being able to stream them. Oof :(

[yuvipanda]

As for testing, I suggest:

1. Create a simple eventstream emitting server in https://github.com/jupyterhub/jupyter-server-proxy/tree/main/tests/resources (similar to the websocket one perhaps)
2. Create tests in test_proxy similar to the websocket tests (https://github.com/jupyterhub/jupyter-server-proxy/blob/234a192793a6a46bc8df18f730ea0060e6d0c3e8/tests/test_proxies.py#L346).

This can be simpler than websockets, because eventstreams are simpler.

[yuvipanda]

ok, I've done one round of review @ganisback! Hope this helps.

@manics I think we should definitely add this feature! Ideally, I'd have liked us to make everything streaming, but that's incompatible with RewriteableResponse :( So making at least eventstreams (which by definition need to be streaming) streaming (and hence incompatible with RewriteableResponse) seems ok to me.

[ganisback]

OK, will address your comments in the weekend

[yuvipanda]

Hey @ganisback! Just wanted to check to see if there's anything else I can do to help you move this forward :) I just ran into this again in a different project!

[ganisback]

Hi @yuvipanda , I am blocking in testing case, can you help add the testing case?

[yuvipanda]

@ganisback it was slightly tricky, but I think I have a useful test for you at https://github.com/ganisback/jupyter-server-proxy/pull/1! It fails on main but passes on this branch!

LMK what you think of it. I haven't had time to look at your changes yet unfortunately but will do this coming week!

[ganisback]

@yuvipanda I increased resp time and added resp data checking, now the tests passed.

[ganisback]

@yuvipanda any other comments?

[krassowski]

Excited for this! Spent to much of my Saturday debugging why SSE was connected but not sending anything.

[krassowski]

@yuvipanda @manics is there anything else that needs to be done here to get it merged?

[krassowski]

Hi folks, just circling back here, is there any way I could help with this PR?

[ganisback]

Hi folks, just circling back here, is there any way I could help with this PR?

I'm just waiting for they merge this PR

[yuvipanda]

Sorry for dropping this, travel / vacation time :( I'll try to spend an hour on this later this week.

[krassowski]

It looks like pre-commit is failing because this PR is behind main branch as of now, for example it does not include the rawsocket.py file which was added last month (https://github.com/jupyterhub/jupyter-server-proxy/blob/main/jupyter_server_proxy/rawsocket.py)

As per https://results.pre-commit.ci/run/github/71295164/1723882819.1ODn2EWuTr2D6HI5NFt0pQ

[krassowski]

I opened a PR against this branch (https://github.com/ganisback/jupyter-server-proxy/pull/2) to fix pre-commit checks

[yuvipanda]

I still don't like using regexes for parsing headers, but I this is a clear improvement over status quo. So I've rebased this, and will merge. Apologies for the long time this one took, @ganisback!

I've opened https://github.com/jupyterhub/jupyter-server-proxy/issues/498 to handle removing the regexes.

[krassowski]

Amazing, thanks so much! Do you plan to cut a new release with it now or only after https://github.com/jupyterhub/jupyter-server-proxy/issues/498 gets addressed?

[yuvipanda]

498 doesn't need to block a release!

[yuvipanda]

If you make a PR following https://github.com/jupyterhub/jupyter-server-proxy/blob/main/RELEASE.md i'm happy to merge that :)

[yuvipanda]

Although i'm off from monday!