langfuse/langfuse-python

Failure to format api errors

Closed this issue · 5 comments

alexcannan commented

I'm having trouble trying out langfuse due to some error formatting bad status codes in this block:

langfuse-python/langfuse/api/resources/trace/client.py

Lines 53 to 64 in bcb3cef

if 200 <= _response.status_code < 300:
return pydantic.parse_obj_as(Trace, _response.json()) # type: ignore
if _response.status_code == 400:
raise Error(pydantic.parse_obj_as(str, _response.json())) # type: ignore
if _response.status_code == 401:
raise UnauthorizedError(pydantic.parse_obj_as(str, _response.json())) # type: ignore
if _response.status_code == 403:
raise AccessDeniedError(pydantic.parse_obj_as(str, _response.json())) # type: ignore
if _response.status_code == 405:
raise MethodNotAllowedError(pydantic.parse_obj_as(str, _response.json())) # type: ignore
if _response.status_code == 404:
raise NotFoundError(pydantic.parse_obj_as(str, _response.json())) # type: ignore

  1. httpx.Response.json() surprisingly returns a dict object, not str (meanwhile, it's typed as Any)

https://github.com/encode/httpx/blob/053bc57c3799801ff11273dd393cb0715e63ecf9/httpx/_models.py#L756

  1. This causes pydantic.parse_obj_as(str, _) to fail:
pydantic.error_wrappers.ValidationError: 1 validation error for ParsingModel[str]
__root__
  str type expected (type=type_error.str)

It looks like this pattern is used a lot through the python sdk, I found 110 instances of "parse_obj_as(str".

Dev-Khant commented

@marcklingen @maxdeichmann Does this need to change?

maxdeichmann commented

Hi @Dev-Khant and @alexcannan, sorry for being late here. This is something i wanted to take a look at, because it woudl definitely increase DX when using the Python library. Some background here:

  • We have a JS/TS and Python package and both of them talk to the same API on the backend
  • In the main repo, we have Fern definitions for our API, which then generates OpenAPI docs as well as Python code for that API
  • The TS SDK depends on the OpenAPI docs to generate TS code around the API.
  • The Python SDK has the /api folder which contains the Python code generated by Fern. All the pydantic code is generated by Fern.
  • Going forward, this is not the best approach, as we have API definitions in Zod, Fern, OpenAPI at the moment. We ended up with this setup as we did not want to change Python code every time we add a field to the API. At the same time, Python code generators from OpenAPI docs did not work well.

What are the ways forward?

  • Make changes to the Pydantic code, as you propose, but then we would not be able to re-generate Python code with fern as it would overwrite our changes.
  • Look into Fern and try to improve error handling. Unfortunately, we use the hosted version of Fern, i am not sure in how far you can use Fern as well. Creating the code would obv. be required to iterate on that
  • Completely remove the the Fern code and replace it with some other code wrappers.

@alexcannan and @Dev-Khant do you have opinions on that? I just asked Fern in how far the community can use Fern as well. Does anyone of you have experience with generating python code from OpanAPI docs?

Dev-Khant commented

I don't have any experience for generating python code from OpenAPI docs. But I can look into it

alexcannan commented

Hmm, I would suggest raising this to the fern developers. It's surprising that their python code generation has type: ignore flags, that's an orange flag for me. I can attempt to put together a very minimal SDK for my specific use case using the api directly. Actually, as long as I don't trigger any HTTP exceptions the SDK works as expected.

Thank you for the detailed explanation @maxdeichmann. It seems as long as no HTTP exceptions are raised, this is a non-issue for users. And, since the SDK is generated directly from API definitions, the chance of exceptions is relatively low. I'm happy to close this for now, since it seems to be a fern generation issue rather than a langfuse one, but I'll leave that up to y'all.

maxdeichmann commented

@alexcannan and @Dev-Khant, thank you so much for looking into this. I agree, this is not very high on our Prio list and hence I will close it. Thanks for raising!

  • 👍1