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.

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

Conversation

StreetLamb
Copy link

Update playground tests to check for the correct playground assets path in index.html. #578

Copy link

cla-bot bot commented Apr 1, 2024

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the username @StreetLamb on file. In order for us to review and merge your code, please complete the Individual Contributor License Agreement here https://forms.gle/AQFbtkWRoHXUgipM6 .

This process is done manually on our side, so after signing the form one of the maintainers will add you to the contributors list.

For more details about why we have a CLA and other contribution guidelines please see: https://github.com/langchain-ai/langserve/blob/main/CONTRIBUTING.md.

@cla-bot cla-bot bot added the cla-signed label Apr 1, 2024
@eyurtsev eyurtsev changed the title Add check for valid playground assets path in playground tests WIP: Serve playground from correct route if nested APIrouters within one another Apr 1, 2024
@eyurtsev
Copy link
Collaborator

eyurtsev commented Apr 1, 2024

@StreetLamb there are some other changes required to make this work, so i can't merge this yet, you can try installing off of this PR if that's helpful

Comment on lines +1568 to +1576
# 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
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

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!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants