-
Notifications
You must be signed in to change notification settings - Fork 979
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
fix: FastAPI built-in paths bypass custom routing (Docs) and update r… #1841
base: main
Are you sure you want to change the base?
Conversation
…equirements document
Hi @solaius! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Left my local file version of llama stack in the requirements.txt file, just updated for >= 0.1.9
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.
Thanks for the improvements. A couple of comments.
openapi_url="/openapi.json", | ||
title="Llama Stack API", | ||
description="API for Llama Stack", | ||
version="0.1.9" |
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.
Let’s not hardcode the version - we should use a version file instead.
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.
removed unnecessary entries.
|
||
async def __call__(self, scope, receive, send): | ||
if scope.get("type") == "lifespan": | ||
return await self.app(scope, receive, send) | ||
|
||
path = scope.get("path", "") | ||
|
||
# Check if the path is a FastAPI built-in path | ||
if any(path.startswith(fastapi_path) for fastapi_path in self.fastapi_paths): |
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.
Why startswith?
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.
Hello! I used startswith to handle routes like any other related subpaths that FastAPI may have. I can remove if you feel it is not worth covering for that.
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.
Also, if you make self.fastapi_paths
a tuple you can just do if path.startswith(self.fastapi_paths):
I think?
Removed 3 unnecessary FastAPI entries
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.
Please do not edit requirements.txt
manually. If new dependencies are needed, please add them to pyproject.toml
and let pre-commit do the rest. Thanks!
Reverted requirements.txt to be same as current main. requirements from initial push had llama-stack-client at 0.1.19 instead of 0.1.9 so I used the latest one. |
requirements.txt
Outdated
@@ -21,7 +21,7 @@ idna==3.10 | |||
jinja2==3.1.6 | |||
jsonschema==4.23.0 | |||
jsonschema-specifications==2024.10.1 | |||
llama-stack-client==0.1.19 | |||
llama-stack-client==0.1.9 |
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.
no changes required here
|
||
async def __call__(self, scope, receive, send): | ||
if scope.get("type") == "lifespan": | ||
return await self.app(scope, receive, send) | ||
|
||
path = scope.get("path", "") | ||
|
||
# Check if the path is a FastAPI built-in path | ||
if any(path.startswith(fastapi_path) for fastapi_path in self.fastapi_paths): |
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.
Also, if you make self.fastapi_paths
a tuple you can just do if path.startswith(self.fastapi_paths):
I think?
Adding logger per request. Co-authored-by: Sébastien Han <[email protected]>
Adding logger per request. Co-authored-by: Sébastien Han <[email protected]>
@leseb you were correct! Thanks for the efficiency :-) |
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.
We still need to remove the requirements.txt
26047e8
to
80adc42
Compare
What does this PR do?
This PR improves the server's request routing logic by ensuring built-in FastAPI paths such as
/docs
,/redoc
,/openapi.json
,/favicon.ico
, and/static
bypass the customTracingMiddleware
. This prevents unnecessary tracing logic for documentation and static file requests, ensuring better performance and cleaner logs.Additionally, it adds proper metadata (
title
,description
, andversion
) to the FastAPI application initialization and updates the requirements document accordingly.Test Plan
uvicorn
using the providedrun.yaml
config/docs
,/redoc
) load correctly without triggering the custom tracing middlewareTo reproduce:
python server.py --template <template-name>
/docs
and/redoc
x-trace-id
in the response headersFroze the requirements file to include many of the other libraries that have been added in the past few releases to make install easier.