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

Failure to format api errors #55

Closed
alexcannan opened this issue Aug 31, 2023 · 5 comments
Closed

Failure to format api errors #55

alexcannan opened this issue Aug 31, 2023 · 5 comments

Comments

@alexcannan
Copy link

alexcannan commented Aug 31, 2023

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

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
Copy link
Contributor

Dev-Khant commented Sep 3, 2023

@marcklingen @maxdeichmann Does this need to change?

@maxdeichmann
Copy link
Member

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
Copy link
Contributor

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

@alexcannan
Copy link
Author

alexcannan commented Sep 11, 2023

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
Copy link
Member

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

@maxdeichmann maxdeichmann closed this as not planned Won't fix, can't repro, duplicate, stale Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants