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.

Sign up for GitHub

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

Jump to bottom

[BugFix] - Untyped variadic keyword arguments break during execution #6250

Merged
merged 20 commits into from
Mar 25, 2024
Merged

[BugFix] - Untyped variadic keyword arguments break during execution #6250

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
921abf2
fix: ensure var_kw args come last in signatures
montezdesousa Mar 22, 2024
6657a9e
remove package builder change
montezdesousa Mar 22, 2024
41befa7
Update package_builder.py
montezdesousa Mar 22, 2024
c932a1b
fix: warn only when ExtraParams
montezdesousa Mar 22, 2024
8c1a67f
track VAR_KEYWORD
montezdesousa Mar 22, 2024
1e0b4e8
minor fix
montezdesousa Mar 22, 2024
248b1b3
type Any if no type provided
montezdesousa Mar 22, 2024
d8f5e5e
minor fix
montezdesousa Mar 22, 2024
7726ff6
Merge branch 'develop' into bugfix/endpoint_kwargs
deeleeramone Mar 22, 2024
182698f
fix test
montezdesousa Mar 22, 2024
7ff02dd
fix: _as_dict
montezdesousa Mar 22, 2024
fc62740
ruff
montezdesousa Mar 22, 2024
da37bca
update reorder_params unit test
montezdesousa Mar 25, 2024
5d42683
update func default and tests
montezdesousa Mar 25, 2024
5c553cf
typing
montezdesousa Mar 25, 2024
db2bcf1
Merge branch 'develop' into bugfix/endpoint_kwargs
montezdesousa Mar 25, 2024
e800fb8
update comment
montezdesousa Mar 25, 2024
7dfb22b
update comment
montezdesousa Mar 25, 2024
01d5326
rename var
montezdesousa Mar 25, 2024
6d49ae6
Update command_runner.py
montezdesousa Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 17 additions & 8 deletions openbb_platform/core/openbb_core/api/router/commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,14 @@ def build_new_signature(path: str, func: Callable) -> Signature:
parameter_list = sig.parameters.values()
return_annotation = sig.return_annotation
new_parameter_list = []

for parameter in parameter_list:
var_kw_start = len(parameter_list)
for pos, parameter in enumerate(parameter_list):
if parameter.name == "cc" and parameter.annotation == CommandContext:
continue

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

Copy link
Contributor

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".

new_parameter_list.append(
Parameter(
parameter.name,
Expand All @@ -66,39 +69,45 @@ def build_new_signature(path: str, func: Callable) -> Signature:
)

if CHARTING_INSTALLED and path.replace("/", "_")[1:] in Charting.functions():
new_parameter_list.append(
new_parameter_list.insert(
var_kw_start,
Parameter(
"chart",
kind=Parameter.POSITIONAL_OR_KEYWORD,
default=False,
annotation=bool,
)
),
)
var_kw_start += 1

if custom_headers := SystemService().system_settings.api_settings.custom_headers:
for name, default in custom_headers.items():
new_parameter_list.append(
new_parameter_list.insert(
var_kw_start,
Parameter(
name.replace("-", "_"),
kind=Parameter.POSITIONAL_OR_KEYWORD,
default=default,
annotation=Annotated[
Optional[str], Header(include_in_schema=False)
],
)
),
)
var_kw_start += 1

if Env().API_AUTH:
new_parameter_list.append(
new_parameter_list.insert(
var_kw_start,
Parameter(
"__authenticated_user_settings",
kind=Parameter.POSITIONAL_OR_KEYWORD,
default=UserSettings(),
annotation=Annotated[
UserSettings, Depends(AuthService().user_settings_hook)
],
)
),
)
var_kw_start += 1

return Signature(
parameters=new_parameter_list,
Expand Down
8 changes: 5 additions & 3 deletions openbb_platform/core/openbb_core/app/command_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from openbb_core.app.model.obbject import OBBject
from openbb_core.app.model.system_settings import SystemSettings
from openbb_core.app.model.user_settings import UserSettings
from openbb_core.app.provider_interface import ProviderInterface
from openbb_core.app.provider_interface import ExtraParams, ProviderInterface
from openbb_core.app.router import CommandMap
from openbb_core.app.service.system_service import SystemService
from openbb_core.app.service.user_service import UserService
Expand Down Expand Up @@ -189,9 +189,11 @@ def _warn_kwargs(
annotation = getattr(
model.model_fields.get("extra_params", None), "annotation", None
)
if annotation:
if is_dataclass(annotation) and any(
t is ExtraParams for t in getattr(annotation, "__bases__", [])
):
# When there is no annotation there is nothing to warn
valid = asdict(annotation()) if is_dataclass(annotation) else {} # type: ignore
valid = asdict(annotation()) # type: ignore
provider = provider_choices.get("provider", None)
for p in extra_params:
if field := valid.get(p):
Expand Down