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

include missing from FHIRAbstractModel.dict() #90

Closed
RafaelWO opened this issue Jan 11, 2022 · 13 comments
Closed

include missing from FHIRAbstractModel.dict() #90

RafaelWO opened this issue Jan 11, 2022 · 13 comments

Comments

@RafaelWO
Copy link

RafaelWO commented Jan 11, 2022

  • fhir.resources version: 6.2
  • Python version: 3.8
  • Operating System: Linux

Additional libraries

  • fastapi: 0.68.1

Description

I use the pydantic model from fhir.resources as response models in FastAPI endpoints, e.g.

from fhir.resources.patient import Patient
...

@router.get("/patients/{pat_id}", response_model=Patient, response_model_exclude_none=True)
def read_patient_by_id(pat_id: str, client=Depends(fhir_client)):
  ...

What I Did

After upgrading to fhir.resources version 6.2, I get the following error at this endpoint

TypeError: dict() got an unexpected keyword argument 'include'
Click for full stack trace
Traceback (most recent call last):
  File "/path/to/python/env/lib/python3.8/site-packages/uvicorn/protocols/http/httptools_impl.py", line 398, in run_asgi
    result = await app(self.scope, self.receive, self.send)
  File "/path/to/python/env/lib/python3.8/site-packages/uvicorn/middleware/proxy_headers.py", line 45, in __call__
    return await self.app(scope, receive, send)
  File "/path/to/python/env/lib/python3.8/site-packages/fastapi/applications.py", line 199, in __call__
    await super().__call__(scope, receive, send)
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/applications.py", line 111, in __call__
    await self.middleware_stack(scope, receive, send)
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/middleware/errors.py", line 181, in __call__
    raise exc from None
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/middleware/errors.py", line 159, in __call__
    await self.app(scope, receive, _send)
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/exceptions.py", line 82, in __call__
    raise exc from None
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/exceptions.py", line 71, in __call__
    await self.app(scope, receive, sender)
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/routing.py", line 566, in __call__
    await route.handle(scope, receive, send)
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/routing.py", line 227, in handle
    await self.app(scope, receive, send)
  File "/path/to/python/env/lib/python3.8/site-packages/starlette/routing.py", line 41, in app
    response = await func(request)
  File "/path/to/python/env/lib/python3.8/site-packages/fastapi/routing.py", line 209, in app
    response_data = await serialize_response(
  File "/path/to/python/env/lib/python3.8/site-packages/fastapi/routing.py", line 127, in serialize_response
    return jsonable_encoder(
  File "/path/to/python/env/lib/python3.8/site-packages/fastapi/encoders.py", line 104, in jsonable_encoder
    jsonable_encoder(
  File "/path/to/python/env/lib/python3.8/site-packages/fastapi/encoders.py", line 47, in jsonable_encoder
    obj_dict = obj.dict(
TypeError: dict() got an unexpected keyword argument 'include'

I guess this is related to the 6.2.0b1 release:

Breaking

  • FHIRAbstractModel.json() and FHIRAbstractModel.dict() parameters signatures are more FHIR specific and additional parameters are removed (pydantic specific).

Relates to #89

@nazrulworld
Copy link
Owner

@RafaelWO sorry to hear about your problem. Can you please describe your usecase of using 'include'. If it is not conflict with fhir specification, we could our best try to keep this parameter.

@RafaelWO
Copy link
Author

RafaelWO commented Jan 12, 2022

I'm not doing this myself, this is part of FastAPI - in this case it is trying to serialize the model to JSON.

Here is the line where the exception raises.

@nazrulworld
Copy link
Owner

Understand that part. Let's see, I have an idea like this 1.) dict() method could accept any pydantic related arguments with doing no action of those extra arguments. 2.) we could do some warning log instead for those extra arguments.

@RafaelWO
Copy link
Author

RafaelWO commented Jan 13, 2022

1.) dict() method could accept any pydantic related arguments with doing no action of those extra arguments. 2.) we could do some warning log instead for those extra arguments.

Sounds good! A warning would be very helpful indeed because it took me a while to figure out what the cause of this error was 🧐

So am I right that it is intended that the resources do not support those pydantic features, e.g. exclude_none, anymore?

@nazrulworld
Copy link
Owner

by_alias & exclude_none are part of FHIRAbstractModel.dict() and with additional encoder argument are part of FHIRAbstractModel.json()

nazrulworld added a commit that referenced this issue Jan 14, 2022
…pydnatic specific extra argument has been provided) is neutralized.
@nazrulworld
Copy link
Owner

nazrulworld commented Jan 14, 2022

The new release v6.2.1 should solve this issue.

@RafaelWO
Copy link
Author

RafaelWO commented Mar 4, 2022

@nazrulworld sorry to bother you again but I experience a ton of the new warnings in my application:

Observation.dict method accepts only´by_alias´, ´exclude_none´, ´exclude_comments` as parameters since version v6.2.0, any extra parameter is simply ignored. You should not provide any extra argument.
Observation.dict method accepts only´by_alias´, ´exclude_none´, ´exclude_comments` as parameters since version v6.2.0, any extra parameter is simply ignored. You should not provide any extra argument.
...

As outlined above, the reason for this is getting those is using the FHIR models as response_model in a FastAPI endpoint. Within FastAPI, the .dict() method is called with all pydantic args (even if I only use exclude_none - others are False or None) here.

Do you think this can be "fixed" from your side? E.g. issuing the warning only once? Or do you not see this issue being within the scope for this package?

For now my workaround is setting the loglevel to error for the corresponding logger:

logging.getLogger('fhir.resources.fhirabstractmodel').setLevel(logging.ERROR)

@nazrulworld
Copy link
Owner

I understand the problem, One thing I could do is that instead of a warning log, do debug log. But also can possible to find a way to warn one time.
Not sure what will be the best solution, but I am really want to let the user/developer know this message.

@nazrulworld nazrulworld reopened this Mar 4, 2022
@RafaelWO
Copy link
Author

RafaelWO commented Mar 7, 2022

Thanks! I would prefer a one-time warning ;)

but I am really want to let the user/developer know this message.

Sure, I understand that! What is the default log level? Maybe logging it as INFO is also an option?

@dajaffe
Copy link

dajaffe commented Nov 6, 2022

Hi @nazrulworld - just came across this in a new FastAPI service I'm working on, using these models as the model responses. To avoid spamming our logs I've had to disable loggers coming from fhir.resources.code. Huge +1 to making the warning a one-time only or downgrading to INFO / DEBUG

@blazing-gig
Copy link

Hey @nazrulworld is there any update on this where the warning can be made only once ? Also, I'm not using FastAPI response models. The logs can be seen just by calling model_instance.json(exclude_none=True) explicitly from code. Ideally I don't expect to see any warning at all if explicitly called this way. Let me know your thoughts.

@nazrulworld
Copy link
Owner

I fully agree with your concerns. I am thinking, maybe is good if we change the log from a warning to debug level.

@janwijbrand
Copy link

janwijbrand commented Sep 28, 2023

Re-opening this issue: the change of log level was only applied to the json() call, yet it is also prominent in the dict() call and as a result still "spams" my log. As an alternative approach I proppose to use the warnings API instead. I made a PR #138 for that. I'm eager to hear what you and other developers using fhir.resources think of this alternative approach.

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

5 participants