Skip to content
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.

Sign up for GitHub

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

WIP: Serve playground from correct route if nested APIrouters within one another #580

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

WIP: Serve playground from correct route if nested APIrouters within one another #580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
81aa1b1
Add check for valid playground assets path in playground tests
StreetLamb Apr 1, 2024
eee5b7e
Merge branch 'main' into add-playground-test
eyurtsev Apr 1, 2024
9d5e1d0
x
eyurtsev Apr 1, 2024
e74898f
x
eyurtsev Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions langserve/api_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1565,11 +1565,27 @@ async def playground(
self._run_name, user_provided_config, request
)

# Fetching the playground URL using information in the route
# to support a case when the APIRouter is nested inside another
# APIRouter.
# In this case the prefix attribute of the APIRouter is not enough
# to get the correct playground URL.
route = request.scope["route"]
suffix = "/{file_path:path}"
route_path = route.path
full_suffix = "/playground" + suffix
Comment on lines +1568 to +1576
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems like it can fix this issue. I've deployed the langserve application on AWS Lambda and set up an API Gateway to access the Lambda function. So, the URL access looks like this: https://api-id.execute-api.region.amazonaws.com/stage/model. Notice that there's a stage prefix in the URL before the path configured by langserve. Consequently, the langserve application isn't aware of the prefix. It will work correctly if I access the model using the invoke method, but it won't work if I access the playground. It will throw a network error because it tries to access assets from a path without the stage prefix.

Copy link

@jianzs jianzs Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eyurtsev Hi there! I couldn't find the reason why the CI failed. Could you please provide some information on that? I'm happy to assist with merging this PR. I tested with Python 3.10 locally. All the tests passed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some odd interaction between pytest and asyncio that's causing unit tests to hang occassionally. I haven't had time to address it yet.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(it's not related to your PR!)


if not route_path.endswith(full_suffix):
raise AssertionError(
f"Expected route to end with " f"'{full_suffix}'. Got: {route.path}"
)

# path without the suffix
playground_url = (
request.scope.get("root_path", "").rstrip("/")
+ self._base_url
+ "/playground"
+ route_path[: -len(f"{suffix}")]
)

feedback_enabled = tracing_is_enabled() and self._enable_feedback_endpoint
public_trace_link_enabled = (
tracing_is_enabled() and self._enable_public_trace_link_endpoint
Expand Down
7 changes: 7 additions & 0 deletions tests/unit_tests/test_api_playground.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ async def test_serve_playground_with_api_router() -> None:
async with AsyncClient(app=app, base_url="http://localhost:9999") as client:
response = await client.get("/langserve_runnables/chat/playground/index.html")
assert response.status_code == 200
assert (
'src="/langserve_runnables/chat/playground/assets/'
in response.content.decode()
), "html should contain reference to valid playground assets path"


async def test_serve_playground_with_api_router_in_api_router() -> None:
Expand All @@ -67,6 +71,9 @@ async def test_serve_playground_with_api_router_in_api_router() -> None:
async with AsyncClient(app=app, base_url="http://localhost:9999") as client:
response = await client.get("/parent/bar/foo/playground/index.html")
assert response.status_code == 200
assert (
'src="/parent/bar/foo/playground/assets/' in response.content.decode()
), "html should contain reference to valid playground assets path"


async def test_root_path_on_playground() -> None:
Expand Down
Loading