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] - Untyped variadic keyword arguments break during execution #6250

Merged
merged 20 commits into from
Mar 25, 2024

Conversation

montezdesousa
Copy link
Contributor

@montezdesousa montezdesousa commented Mar 22, 2024

  1. Why?

    • Untyped VAR_KEYORD arguments in endpoints like **kwargs are not supported
    • Warning that checks extra_params is raising by mistake whenever extra_params exists
  2. What?

    • Warn only if extra_params is of type ExtraParams
    • validate_kwargs allows untyped arguments like **kwargs to pass validation as Any
    • In the API signature wrapper make sure that VAR_KEYWORD arguments always come last
    • In the package builder generalises VAR_KEYWORD argument names to any name and makes sure they always come last
  3. Impact

    • We can now use VAR_KEYWORD parameters (**kwargs) in the endpoints, although this is has some non obvious caveats in the API - it will make the VAR_KEYWORD a required URL parameter, since ** parameters cannot have a default, thus FastAPI sees it as required. Will be on the developer side if this is good for the use case or not.
  4. Testing Done:

    • Add extra_params: Optional[Dict] = None to any POST method, rebuild, run commands or make API calls
    • Add **any_arg to an endpoint and run commands or make API calls

@github-actions github-actions bot added bug Fix bug platform OpenBB Platform v4 PRs for v4 labels Mar 22, 2024
@montezdesousa montezdesousa changed the base branch from develop to bugfix/package-builder-kwargs March 22, 2024 12:53
@montezdesousa montezdesousa changed the title fix: ensure var_kw args come last in signatures [BugFix] - ensure var_kw args come last in signatures Mar 22, 2024
@montezdesousa montezdesousa marked this pull request as ready for review March 22, 2024 12:57
@montezdesousa montezdesousa changed the title [BugFix] - ensure var_kw args come last in signatures [BugFix] - Ensure VAR_KEYWORD args come last in signatures Mar 22, 2024
@deeleeramone
Copy link
Contributor

I still get the same error,

Cannot parse: 593:8:         chart: Annotated[bool, OpenBBCustomParameter(description="Whether to create a chart or not, by default False.")] = False

We very much need this in PackageBuilder:

for k in ["provider", "extra_params", "**kwargs"]

We still need changes in CommandRunner:

ValidationError: 1 validation error for cones
kwargs
  Input should be an instance of _empty [type=is_instance_of, input_value={'chart_params': {'tite': 'some param'}}, input_type=dict]
    For further information visit https://errors.pydantic.dev/2.6/v/is_instance_of

There are two things that happen in my proposed changes to CommandRunner:

  • "kwargs" gets popped from the list of parameters that gets validated. This is required so a "kwargs" is not considered as a field for validation.
  • "kwargs" is properly unpacked so that it is flat relative to the rest of the params in the POST function. i.e, not dumped as a field name called, "kwargs".

@montezdesousa montezdesousa changed the base branch from bugfix/package-builder-kwargs to develop March 22, 2024 17:16
if parameter.name == "cc" and parameter.annotation == CommandContext:
continue

if parameter.kind == Parameter.VAR_KEYWORD:
var_kw_start = pos

Copy link
Contributor

@deeleeramone deeleeramone Mar 22, 2024

Choose a reason for hiding this comment

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

"kwargs" is being forced as a "required" field in the body, and calling it VAR_KEYWORD or VAR_POSITIONAL is not allowed with a default value. Optional[Dict] is not actually optional without a default state. Setting as POSITIONAL_OR_KEYWORD allows you to set a default value of {}, and **kwargs is still in the request body.

It would be ideal if you didn't have to put kwargs in the body, but could just add them to the URL like every other parameter. This behaviour, though, is limited to the API. The Python interface does not require them to be declared within the field "kwargs".

        if parameter.name == "kwargs":
            new_parameter_list.append(
                Parameter(
                    parameter.name,
                    kind=parameter.POSITIONAL_OR_KEYWORD,
                    annotation=Optional[Dict[str, Any]],
                    default={},
                )
            )
        else:
            new_parameter_list.append(
                Parameter(
                    parameter.name,
                    kind=parameter.kind,
                    default=parameter.default,
                    annotation=parameter.annotation,
                )
            )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible to add undefined URL parameters to API endpoints

Copy link
Contributor

Choose a reason for hiding this comment

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

Well then we can't get around that "nice to have" thing, but at least they can be unpacked the same as the Python interface.

This: body = json.dumps({"data": data, "kwargs": {"chart_params": {"title": "my chart"}}})

When unpacked becomes the same as the Python Interface "extras":

{'metadata': {'arguments': {'index': 'date',
   'lower_q': 0.25,
   'upper_q': 0.75,
   'model': 'std',
   'is_crypto': False,
   'trading_periods': None,
   'data': {'type': 'List[Data]',
    'columns': ['open',
     'vwap',
     'split_ratio',
     'close',
     'volume',
     'dividend',
     'date',
     'low',
     'high']},
   'chart_params': {'title': 'my chart'}},
  'duration': 109062167,
  'route': '/technical/cones',
  'timestamp': '2024-03-22T12:05:53.906124'}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Metadata reflects the call that was made to the api, if the call sends 'chart_params': {'title': 'my chart'}} inside a parameter called kwargs/some_kw_args/extra_params it is expected that you see

{'metadata': 
{'arguments': 
        {
         'index': 'date',
         'lower_q': 0.25,
         'data': {
                    'type': 'List[Data]',
                    'columns': ['open', ...],
          },
         'kwargs/some_kw_args/extra_params': {'chart_params': {'title': 'my chart'}}
         },
  'duration': 109062167,
  'route': '/technical/cones',
  'timestamp': '2024-03-22T12:05:53.906124'}}

Copy link
Contributor

Choose a reason for hiding this comment

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

For this purpose, "extra_params" is a better name for this than "kwargs". Less confusion with everything else that is passed around as "kwargs".

@montezdesousa montezdesousa changed the title [BugFix] - Ensure VAR_KEYWORD args come last in signatures [BugFix] - Ensure variadic keyword parameters come last in signatures Mar 22, 2024
@montezdesousa montezdesousa changed the title [BugFix] - Ensure variadic keyword parameters come last in signatures [BugFix] - Untyped variadic keyword arguments break during execution Mar 22, 2024
@deeleeramone
Copy link
Contributor

Add extra_params: Optional[Dict] = None to any POST method, rebuid, run commands or make API calls

I get an error when no kwargs are supplied to the POST request:

"'NoneType' object is not iterable"

@montezdesousa
Copy link
Contributor Author

Add extra_params: Optional[Dict] = None to any POST method, rebuid, run commands or make API calls

I get an error when no kwargs are supplied to the POST request:

"'NoneType' object is not iterable"

thanks, _as_dict was buggy

@montezdesousa montezdesousa added this pull request to the merge queue Mar 25, 2024
Merged via the queue into develop with commit 1794d69 Mar 25, 2024
10 checks passed
@IgorWounds IgorWounds deleted the bugfix/endpoint_kwargs branch April 9, 2024 13:35
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