Ssr part 2 #9339

pngwn · 2024-09-12T16:45:55Z

Description

Please include a concise summary, in clear English, of the changes in this pull request. If it closes an issue, please mention it here.

Closes: #(issue)

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests

PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh
You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

freddyaboulton · 2024-09-19T15:40:39Z

gradio/blocks.py

+        self.node_port = None
+
+        if not Blocks.node_process:
+            (node_server_name, node_process, node_port) = start_node_server(


Rather than starting node in init, we can do in startup_events method of gr.Blocks. That way, if you're nesting multiple blocks in a single app, only one node process is started.

And startup_events runs even when mounted in a larger FastAPI app.

Ok I think we're only going to start one node process anyways with this implementation since we're attaching it to the class but I still think startup_events is a bit cleaner.

Yeah @abidlabs suggested that but we can't do it there if i remember from my testing because we need to know certain details before mounting the gradio app.

Happy to change it in another PR if i'm wrong, i just don't have time to test it atm, the process does only start once tho.

i think i might be wrong actually but I'd need to test

abidlabs · 2024-09-19T18:15:27Z

(3) It seems that port 7860 is hardcoded somewhere. If a Gradio app is already running on port 7860, its frontend will be shown on the page, even if the new Gradio app that you launched is running on port 7861, etc.

This is now resolved, nice @pngwn!

abidlabs · 2024-09-19T18:19:26Z

Btw two quick suggestions for the DX:

(1) I highly recommend renaming spa_mode to ssr_mode (and of course, inverting the default value). We're going to be publicizing "server-side rendering", and so users will be looking for an "ssr"-related parameter, it'd be good to use a single term throughout. It also is more intuitive that the default value is False, and a developer needs to set it to True to enable SSR.

(2) I think its far more intuitive for this parameter to live in launch(). I believe the reason we didn't add it there was in the case a developer is mounting a Gradio app to another FastAPI app, but we can add this parameter to mount_gradio_app as well, as we've done with other parameters.

freddyaboulton · 2024-09-19T18:28:20Z

gradio/utils.py

+
+    print("Unable to find node install path, falling back to SPA mode.")
+    print(
+        "If you wish to use the node backend, please install node 18 and/ or set the path with the GRADIO_NODE_PATH environment variable."


I have node 20 installed and ssr works. Should this print statement be modified or are we lucky that it works with node 20?

We should change it to 18+

freddyaboulton

@pngwn awesome PR!

Left a couple of suggestions but will keep on testing. Bigger comments though:

SSR seems to apply different styles than non-ssr mode. For example the Error toast takes up the whole width:

And here is a side-by-side of the same app in ssr and non-ssr mode

SSR

Non-ssr

Everything is stretched out horizontally.

Additionally, SSR mode does not respect my dark mode system setting and it does not respect the ?__theme query parameter either.

freddyaboulton · 2024-09-19T18:45:08Z

gradio/node_server.py

+
+            node_process = subprocess.Popen(
+                [node_path, SSR_APP_PATH],
+                # stdout=subprocess.PIPE,


nit: delete

freddyaboulton · 2024-09-19T18:48:31Z

gradio/routes.py

+        node_port: int,
+        python_port: int,
+    ) -> Response:
+        client = httpx.AsyncClient()


I think we can create one client at the top level of the file and re-use it. It may even be better performance as it will use http connection pooling: https://www.python-httpx.org/advanced/clients/

I tried that but it was erroring that i can't use a closed client or something.

Oh wait thats because of the with usage, if i change that, it will work.

yvrjsharma · 2024-09-19T19:03:20Z

TYSM @pngwn, I can now confirm that I am able to build multiple demos successfully on the Windows machine, both with and without specifying spa_mode=False. Absolute legend!

pngwn · 2024-09-19T19:49:12Z

Regarding 2, I'm not sure if we can. I think we create the app really early on and I need to know whether to attach the middleware when we create the app but I'll check.

abidlabs · 2024-09-20T04:26:08Z

Let me see if I can put together a PR with my suggestions

Edit: PR ready here: #9399

abidlabs · 2024-09-20T05:14:55Z

Fixing the issue around share links is more interesting. It appears to be a CORS issue since access to localhost links is blocked from external links, including gradio.live links, which would be an easy fix. But I actually think there's a deeper issue, which is that xxxx.gradio.live links are trying to fetch the config from a user's localhost url instead of using the relative xxxx.gradio.live/config url. Happy to sync with you on this afterwards @pngwn but I don't think its a blocker for the beta release

pngwn · 2024-09-20T05:28:17Z

Ah. The app just uses whatever the 'api' is listed as to get those links. Can sync later.

abidlabs · 2024-09-20T06:25:08Z

Python unit tests should be passing now

pngwn and others added 30 commits August 27, 2024 17:31

start

7fb0c9b

add changeset

1e6ba74

lint

c6db5b3

clean up

4b25fcb

Merge branch '5.0-dev' into prefix

f78f92e

more

90a59ca

fix imports

791e2b7

fix more

6e9e40d

Merge branch '5.0-dev' into prefix

5ebe6cc

lint js

f97bee6

format

358a847

add changeset

c8640ce

remove route config

11d5ba7

Merge branch '5.0-dev' into prefix

4ee634b

Merge branch '5.0-dev' into prefix

69ff405

Merge branch '5.0-dev' into prefix

393a0d2

update js client to use api prefix from config

0248cf2

ensure api prefix is always factored in where appropriate

7966d9c

ensure correct local url for streaming

23f1b5c

use correct prefix when constructing file urls on frontend

2ff8ae6

fix api docs + recorder

f7e1cde

add changeset

500049e

fix component loading by url

19201c9

remove files

15aabbe

linter

ae684b8

fix test chatbot

1f4025a

more fixes

be1521d

refactor

67ff7e9

refactor

068392f

refactor

0467171

freddyaboulton reviewed Sep 19, 2024

View reviewed changes

pngwn and others added 4 commits September 20, 2024 00:48

fix

002f48a

Merge branch '5.0-dev' into ssr-part-2

a2d11fb

add changeset

d7b5465

asd

a0f55c5

freddyaboulton reviewed Sep 19, 2024

View reviewed changes

pngwn added 2 commits September 20, 2024 03:11

asd

58c7048

fix

ee3143e

abidlabs added 2 commits September 20, 2024 13:26

Tweaks to SSR mode (#9399)

bf329b0

leftover format

c2cae1d

abidlabs added 11 commits September 20, 2024 00:28

notebooks

9c07fc3

Merge branch '5.0-dev' into ssr-part-2

99d3180

notebooks

bcba342

login/logout fixes

972c1f6

change logout to be at an app level

263c830

test routes

c9446f6

fix monitoring

d1a5fbc

routes

5e1926a

more tests

a6431cf

format

fa2762c

comment test that is passing locally

0c41783

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ssr part 2 #9339

Ssr part 2 #9339

pngwn commented Sep 12, 2024

freddyaboulton Sep 19, 2024

freddyaboulton Sep 19, 2024

pngwn Sep 19, 2024

pngwn Sep 19, 2024

pngwn Sep 19, 2024

abidlabs commented Sep 19, 2024

abidlabs commented Sep 19, 2024 •

edited

Loading

freddyaboulton Sep 19, 2024

pngwn Sep 19, 2024

freddyaboulton left a comment •

edited

Loading

freddyaboulton Sep 19, 2024

freddyaboulton Sep 19, 2024

pngwn Sep 19, 2024

pngwn Sep 19, 2024

yvrjsharma commented Sep 19, 2024

pngwn commented Sep 19, 2024

abidlabs commented Sep 20, 2024 •

edited

Loading

abidlabs commented Sep 20, 2024 •

edited

Loading

pngwn commented Sep 20, 2024

abidlabs commented Sep 20, 2024

Ssr part 2 #9339

Are you sure you want to change the base?

Ssr part 2 #9339

Conversation

pngwn commented Sep 12, 2024

Description

🎯 PRs Should Target Issues

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidlabs commented Sep 19, 2024

abidlabs commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddyaboulton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvrjsharma commented Sep 19, 2024

pngwn commented Sep 19, 2024

abidlabs commented Sep 20, 2024 • edited Loading

abidlabs commented Sep 20, 2024 • edited Loading

pngwn commented Sep 20, 2024

abidlabs commented Sep 20, 2024

abidlabs commented Sep 19, 2024 •

edited

Loading

freddyaboulton left a comment •

edited

Loading

abidlabs commented Sep 20, 2024 •

edited

Loading

abidlabs commented Sep 20, 2024 •

edited

Loading