feat: allow root path to run the app on `/path` #14972

pritamsoni-hsr · 2022-10-02T16:40:02Z

What does this PR do?

allow apps to be served from a custom path when running app behind a proxy.

by default apps are served from https://<app_id>.litng-ai-03.litng.ai, this PR adds a feature so apps can be served from 'https://mydomain.com/<my_app>'.

if you want to build custom UI, make sure all internal links are prefixed with window.app_prefix

this works only behind a proxy and not on the api_id.litng-ai-03.litng.ai/<custom_path> and this is because of uvicorn's root path param.

Limitations

relative links inside the app will break, so user should use window.app_prefix in their JS, there is no warning around that atm.

breaking changes

none

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

…i-hsr/lightning into pritam/add-path-prefix

hhsecond · 2022-10-06T14:15:46Z

relative links inside the app will break, so user should use window.app_prefix in their JS, there is no warning around that atm.

Can this be updated in the docs?

hhsecond

Some minor nits and clarification on the docs

src/lightning_app/core/app.py

src/lightning_app/utilities/frontend.py

tchaton

LGTM!
@pritamsoni-hsr Do you think it would make sense to extract those metadata directly into a private file under a .lightning folder?

Borda · 2022-10-06T16:14:41Z

Do you think it would make sense to extract those metadata directly into a private file under a .lightning folder?

let's leave it s separate feature :)

src/lightning_app/core/app.py

src/lightning_app/frontend/web.py

awaelchli · 2022-10-06T22:12:22Z

src/lightning_app/frontend/frontend.py

        """Start the process that serves the UI at the given hostname and port number.

        Arguments:
            host: The hostname where the UI will be served. This gets determined by the dispatcher (e.g., cloud),
                but defaults to localhost when running locally.
            port: The port number where the UI will be served. This gets determined by the dispatcher, which by default
                chooses any free port when running locally.
-
+            root_path: root_path for the server if app in exposed via a proxy at `/<root_path>`


Suggested change

root_path: root_path for the server if app in exposed via a proxy at `/<root_path>`

root_path: root_path for the server if app in exposed via a proxy at `/<root_path>`

You need to leave an empty line

Can we include an example like https://domain.com/<root_path>/my_page to visualize what it means.

tests/tests_app/frontend/test_web.py

Co-authored-by: Adrian Wälchli <[email protected]>

for more information, see https://pre-commit.ci

pritamsoni-hsr · 2022-10-07T13:08:49Z

@tchaton we could, but I think maintaining it in the app is easiest, we can later serialize and deserialize .lightning file

…i-hsr/lightning into pritam/add-path-prefix

* feat: add base path * uvicorn fix arg * Add prefix * update with base_path fix * replace base path with root path * Apply suggestions from code review Co-authored-by: Kaushik B <[email protected]> Co-authored-by: Kaushik B <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 8008055)

* feat: add base path * uvicorn fix arg * Add prefix * update with base_path fix * replace base path with root path * Apply suggestions from code review Co-authored-by: Kaushik B <[email protected]> Co-authored-by: Kaushik B <[email protected]> Co-authored-by: Jirka Borovec <[email protected]> Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

github-actions bot added the app (removed) Generic label for Lightning App package label Oct 2, 2022

pritamsoni-hsr added 2 commits October 4, 2022 14:22

feat: add base path

feb84f9

uvicorn fix arg

4cc4316

Borda added the priority: 0 High priority task label Oct 5, 2022

Borda added this to the app:0.6.x milestone Oct 5, 2022

Borda mentioned this pull request Oct 5, 2022

Update for Muse #15005

Merged

12 tasks

kaushikb11 and others added 6 commits October 6, 2022 12:25

Merge branch 'master' into pritam/add-path-prefix

e836761

Add prefix

ec3b0ac

Fix tests

f1099c2

update with base_path fix

385851e

Merge branch 'pritam/add-path-prefix' of https://github.com/pritamson…

871c735

…i-hsr/lightning into pritam/add-path-prefix

replace base path with root path

89875c7

kaushikb11 changed the title ~~feat: add base path~~ feat: add root path Oct 6, 2022

kaushikb11 added 3 commits October 6, 2022 19:06

update frontend utility

ee3ece7

fix function name

e9be5a7

fix mypy

7502fca

pritamsoni-hsr marked this pull request as ready for review October 6, 2022 14:06

pritamsoni-hsr requested review from tchaton, awaelchli, manskx, hhsecond and lantiga as code owners October 6, 2022 14:06

pritamsoni-hsr marked this pull request as draft October 6, 2022 14:06

Merge branch 'master' into pritam/add-path-prefix

31db1bf

pritamsoni-hsr changed the title ~~feat: add root path~~ feat: allow root path to run the app on /path Oct 6, 2022

fix tests

4c20797

kaushikb11 marked this pull request as ready for review October 6, 2022 14:17

hhsecond reviewed Oct 6, 2022

View reviewed changes

src/lightning_app/core/app.py Show resolved Hide resolved

src/lightning_app/utilities/frontend.py Outdated Show resolved Hide resolved

src/lightning_app/utilities/frontend.py Outdated Show resolved Hide resolved

tchaton approved these changes Oct 6, 2022

View reviewed changes

Address reviews

8479731

Borda requested review from hhsecond, Borda and kaushikb11 October 6, 2022 22:07

awaelchli reviewed Oct 6, 2022

View reviewed changes

kaushikb11 and others added 8 commits October 7, 2022 09:55

Address reviews

f73c23e

Address reviews

d36a2d9

Update tests

688f8d2

Apply suggestions from code review

fb0b326

Co-authored-by: Adrian Wälchli <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

51b566e

for more information, see https://pre-commit.ci

Update docstrong

be6e584

Merge branch 'master' into pritam/add-path-prefix

6bf63dc

[pre-commit.ci] auto fixes from pre-commit.com hooks

2cacac9

for more information, see https://pre-commit.ci

ethanwharris approved these changes Oct 7, 2022

View reviewed changes

mergify bot added the ready PRs ready to be merged label Oct 7, 2022

manskx approved these changes Oct 7, 2022

View reviewed changes

Borda enabled auto-merge (squash) October 7, 2022 13:07

kaushikb11 added 2 commits October 7, 2022 18:56

Update docstring

2882807

Merge branch 'pritam/add-path-prefix' of https://github.com/pritamson…

1aa5c7a

…i-hsr/lightning into pritam/add-path-prefix

kaushikb11 approved these changes Oct 7, 2022

View reviewed changes

Borda merged commit 8008055 into Lightning-AI:master Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow root path to run the app on `/path` #14972

feat: allow root path to run the app on `/path` #14972

pritamsoni-hsr commented Oct 2, 2022 •

edited

Loading

hhsecond commented Oct 6, 2022 •

edited

Loading

hhsecond left a comment

tchaton left a comment

Borda commented Oct 6, 2022

awaelchli Oct 6, 2022

awaelchli Oct 6, 2022

pritamsoni-hsr commented Oct 7, 2022

	root_path: root_path for the server if app in exposed via a proxy at `/<root_path>`
	root_path: root_path for the server if app in exposed via a proxy at `/<root_path>`

feat: allow root path to run the app on /path #14972

feat: allow root path to run the app on /path #14972

Conversation

pritamsoni-hsr commented Oct 2, 2022 • edited Loading

What does this PR do?

Limitations

breaking changes

Before submitting

PR review

Did you have fun?

hhsecond commented Oct 6, 2022 • edited Loading

hhsecond left a comment

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

Borda commented Oct 6, 2022

awaelchli Oct 6, 2022

Choose a reason for hiding this comment

awaelchli Oct 6, 2022

Choose a reason for hiding this comment

pritamsoni-hsr commented Oct 7, 2022

feat: allow root path to run the app on `/path` #14972

feat: allow root path to run the app on `/path` #14972

pritamsoni-hsr commented Oct 2, 2022 •

edited

Loading

hhsecond commented Oct 6, 2022 •

edited

Loading