-
Notifications
You must be signed in to change notification settings - Fork 92
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
Proxy python app frameworks #4978
Conversation
this helps us avoid mixed content errors and will effectively upgrade us from http --> https when the browser is loaded in a secure context.
some servers may take a few more moments to start the app
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 great @sharon-wang! positron-python
and positron-run-app
code changes look good. I don't know how the ui.py
type error snuck through to main, but I think it is fixed in #4966.
EDIT: Oh it looks like the positron-python
unit tests also need to be updated.
Thanks @seeM! We're seeing all green checkmarks now :) And I've copied over the types from |
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 think someone with more experience in these frameworks should test the changes in detail, but code LGTM modulo a couple of exception handlers. Splitting the proxy creation into two pieces is a cool way to deal with the chicken-and-egg problem.
Thanks for the review @jmcphers! @softwarenerd and I did a live review and we opted to "modernize" the promise handling and I've added some corresponding exception handling. |
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.
LG!
On Windows Desktop, a simple dash app (found here) gets stuck on the "Loading". Here is some out put in the dev console It should just be "Hello world" |
fixes issue on Windows where the regex match in positron-run-app includes ansi escape codes, resulting in the proxy sending requests to the wrong target uri (since it includes some misc. characters).
Integration tests are now passing ✅ The last full run failed on one test, but it is another instance of #4863 based on the screenshot in the run artifacts. Thanks everyone for the reviews and testing! 🙌 |
Description
Framework Support Summary (based on limited testing)
UVICORN_ROOT_PATH
gradio==3.3.1 fastapi==0.85.2 httpx==0.24.1
. See gradio-app/gradio#9529 for more information on why more recent versions don't work.Implementation Notes
Positron Proxy
CC @softwarenerd & @jmcphers
positronProxy.startPendingProxyServer
which starts a new proxy server, but doesn't set up the middleware right away. The command will return theexternalUri
of the proxy server (this is the same as the url shown in the Viewer), theproxyPath
(which some app frameworks use as theurlPrefix
) andfinishProxySetup()
which the command-caller will invoke, passing thetargetOrigin
(the actual app url) so that we can finish setting up the middleware to proxy requests from theexternalUri
(app proxy uri) to thetargetOrigin
(actual app uri).startProxyServer()
will continue to use that method which still does the all-at-once proxy setup, by callingstartNewProxyServer()
andfinishProxySetup()
in successionstartNewProxyServer()
standalone, which will only start up the server and then callfinishProxySetup()
later, once thetargetOrigin
is knownPositron Run App
CC @seeM
localUrlRegex
to include the path after the url port<HOST>:<PORT>/<PATH>
positronProxy.startPendingProxyServer
before setting up terminal options or the debug configuration, so we can start the proxy server and get theurlPrefix
localUri
to the positron proxy viaproxyInfo.finishProxySetup(localUri.toString())
, so that the proxy middleware is set upport
from types and test files as it is unusedPython Extension
CC @seeM
port
from thewebAppCommands
and related test files which is unusedCustom
resolveExternalUri
CC @melissa-barca
resolvedUri
to inherit the protocol used by the main window as the uri's scheme, so we can upgrade tohttps
if the main window is running in a secure contextQA Notes
This PR involves refactoring the positron-proxy, which is used by Help, Interactive Plots and the Viewer. We should not see any regression in these types of proxied content.
Positron Server Web and Desktop should be working across all app frameworks in:
gradio==3.3.1 fastapi==0.85.2 httpx==0.24.1
. If you can get it working with a more recent combination of versions, please let me know!Positron on Workbench:
dash
works if the generated.env
file is deleted before running. Once this issue is complete, this extra step won't be needed.fastapi
does not work. The current hunch is that Workbench settingUVICORN_ROOT_PATH
at session start is interfering with how we run thefastapi
app withuvicorn
. TheUVICORN_ROOT_PATH
is set based on the default fastapi port8000
, however I think we want the proxied app url as the root path, which is provided by the Positron Proxy. But, when setting--root-path
in the fastapi app command to the proxied root path, it does not work in Server Web or Desktop. TBD if this resolves the issue on Workbench. More investigation needed.flask
works as long as you include the code snippet from Workbench docs.gradio
works when these versions are installedgradio==3.3.1 fastapi==0.85.2 httpx==0.24.1
. There's an issue in newer versions of Gradio when the app is run via a proxy.streamlit
does not work when SSL is set up. More investigation needed.shiny
no notes!