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

[BugFix] Set Default State For **kwargs in API to Make Them Optional. #6252

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

deeleeramone
Copy link
Contributor

This Tags on to #6250, #6248, #6240.

When parameter.kind == Parameter.VAR_KEYWORD, it correctly cannot be assigned a default state because **kwargs cannot have one.

However, Fast API does not see it the same way and will make it a required field. In both, the body, and URL parameters.

To get around this, we can set:

parameter.kind == Parameter.POSITIONAL_OR_KEYWORD, then assign it as Optional[Dict[str, Any]], and give it a default state of None.

The end result is that **kwargs can be a valid parameter for the API that gets assigned to the body of the request.

Note that **kwargs could be anything - **extra_params.

@deeleeramone deeleeramone added bug Fix bug platform OpenBB Platform v4 PRs for v4 labels Mar 23, 2024
@deeleeramone deeleeramone changed the base branch from develop to bugfix/endpoint_kwargs March 23, 2024 02:09
@deeleeramone deeleeramone changed the base branch from bugfix/endpoint_kwargs to develop March 25, 2024 20:29
Copy link
Contributor

@hjoaquim hjoaquim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to change FastAPI behavior? I believe it will cause some unexpected results in the future.

There are some references on this matter, namely: fastapi/fastapi#251 and https://stackoverflow.com/questions/59929028/python-fastapi-error-422-with-post-request-when-sending-json-data/70636163#70636163.

Why not go for the simplest approach and have something like this:
(example signature):
def correlation_matrix(data: List[Data], payload: Dict[Any, Any] = None) -> OBBject[List[Data]]:

Which could then be used as follows on a POST request:

{
  "data": [
    {
      "additionalProp1": {}
    }
  ],
  "payload": {"chart_arg_1":"value1","chart_arg_2":"value2"}
}

Which results in having the following values inside the function during execution:

image

Note taht this approach would avoid touching the commands.py file.

@deeleeramone
Copy link
Contributor Author

deeleeramone commented Apr 5, 2024

Why do we need to change FastAPI behavior? I believe it will cause some unexpected results in the future.

We aren't. We simply applying the correct Type and default state to **kwargs, which FastAPI does not recognize, but without having to manipulate the function signature in PackageBuilder, like what currently happens via changing "extra_params" to **kwargs. This does not require manipulating the function signature.

The reason to use **kwargs is to eliminate "unexpected keyword argument", and the added benefit is ANYONE can use **kwargs as a way to extend functionality without having to touch the router. This makes it work like the GET/provider functions where you can already do that.

If you just declare a Field, you then have to touch the router. This accomplishes exactly the same thing as your outcome and allows **extra_params to exist in POST functions where they could not be used before.

Why not go for the simplest approach and have something like this: (example signature): def correlation_matrix(data: List[Data], payload: Dict[Any, Any] = None) -> OBBject[List[Data]]:

Which could then be used as follows on a POST request:

{
  "data": [
    {
      "additionalProp1": {}
    }
  ],
  "payload": {"chart_arg_1":"value1","chart_arg_2":"value2"}
}

Which results in having the following values inside the function during execution:

Note taht this approach would avoid touching the commands.py file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug platform OpenBB Platform v4 PRs for v4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants