michaelfeil / infinity

Infinity is a high-throughput, low-latency REST API for serving vector embeddings, supporting a wide range of text-embedding models and frameworks.
https://michaelfeil.eu/infinity/
MIT License
959 stars 71 forks source link

Add support to pass `root_path` to FastAPI app #261

Closed chiragjn closed 3 weeks ago

chiragjn commented 3 weeks ago

When behind some proxy like nginx, Traefik, Istio, the app might be behind some prefix that is unknown to the app. Without knowing the prefix swagger UI cannot send requests to the correct location. Passing the prefix via root_path corrects the server value in openapi See https://fastapi.tiangolo.com/advanced/behind-a-proxy/


On a side note, I am wondering if there is a way to configure miscellaneous fastapi and uvicorn options via external env or config files instead of having infinity take the burden of passing them along

codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.08%. Comparing base (d325d9a) to head (28fc956).

Files Patch % Lines
libs/infinity_emb/infinity_emb/infinity_server.py 0.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #261 +/- ## ========================================== + Coverage 76.97% 77.08% +0.11% ========================================== Files 34 34 Lines 2397 2400 +3 ========================================== + Hits 1845 1850 +5 + Misses 552 550 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

michaelfeil commented 3 weeks ago

Would it make sense to unify this with “url_prefix“?

I think I made url_prefix this - (making it “/v1“ or “/openai/v1”? Seems there are more features and we should rename url_prefix to do this behaviour?

chiragjn commented 3 weeks ago

I think there is a crucial difference between the two root_path is something that does not affect the routes within the app itself while url_prefix does. Former is invisble to the app and is controlled only by a reverse proxy. For example, we can setup two different infinity deployments on a domain with nginx routes as:

https://example.org/michaelfeil/ started with --url-prefix '' deployed on IP 10.0.0.1, port 8000 and

https://example.org/chiragjn/ started with --url-prefix '/infinity/v1/' deployed on IP 10.0.0.2, port 8000

https://example.org/michaelfeil/models --->  http://10.0.0.1:8000/models -> 200 OK
https://example.org/chiragjn/models --->  http://10.0.0.2:8000/models -> 404
https://example.org/chiragjn/infinity/v1/models --->  http://10.0.0.2:8000/infinity/v1/models -> 200 OK

The only valid reason to pass the prefix /michaelfeil/ or /chiragjn/ as root_path is to make the Swagger UI (openapi.json) proxy aware when it is sending requests

michaelfeil commented 3 weeks ago

Got it, but then I would deprecate url_prefix!

The difference from an external view is that root path also moves /docs and /openapi.json, while url_prefix has become more of a burden parameter that needs to be passed to any route.

Also, do you think root_path is a good param name? Could it be a bit longer / less general?

chiragjn commented 3 weeks ago

Also, do you think root_path is a good param name? Could it be a bit longer / less general?

Don't have a strong opinion here 😅 , just thought to keep the naming from FastAPI as people reaching for this option would probably know what they are doing Feel free to modify it as needed

michaelfeil commented 3 weeks ago

Ah, now I understand fully what the traefik does! This is about the fact that FastAPI gets confused when the route is not what fastapi expects.

LGTM, thanks for bearing with me!