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 all 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 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,16 @@ 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_pos = 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:
# We track VAR_KEYWORD parameter to insert the any additional
# parameters we need to add before it and avoid a SyntaxError
var_kw_pos = pos

Copy link
Contributor

@deeleeramone deeleeramone Mar 22, 2024 •
edited
Loading

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 +71,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_pos,
Parameter(
"chart",
kind=Parameter.POSITIONAL_OR_KEYWORD,
default=False,
annotation=bool,
)
),
)
var_kw_pos += 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_pos,
Parameter(
name.replace("-", "_"),
kind=Parameter.POSITIONAL_OR_KEYWORD,
default=default,
annotation=Annotated[
Optional[str], Header(include_in_schema=False)
],
)
),
)
var_kw_pos += 1

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

return Signature(
parameters=new_parameter_list,
Expand Down
22 changes: 15 additions & 7 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 @@ -185,13 +185,16 @@ def _warn_kwargs(
) -> None:
"""Warn if kwargs received and ignored by the validation model."""
# We only check the extra_params annotation because ignored fields
# will always be kwargs
# will always be there
annotation = getattr(
model.model_fields.get("extra_params", None), "annotation", None
)
if annotation:
# When there is no annotation there is nothing to warn
valid = asdict(annotation()) if is_dataclass(annotation) else {} # type: ignore
if is_dataclass(annotation) and any(
t is ExtraParams for t in getattr(annotation, "__bases__", [])
):
# We only warn when endpoint defines ExtraParams, so we need
# to check if the annotation is a dataclass and child of ExtraParams
valid = asdict(annotation()) # type: ignore
provider = provider_choices.get("provider", None)
for p in extra_params:
if field := valid.get(p):
Expand All @@ -216,7 +219,12 @@ def _warn_kwargs(
@staticmethod
def _as_dict(obj: Any) -> Dict[str, Any]:
"""Safely convert an object to a dict."""
return asdict(obj) if is_dataclass(obj) else dict(obj)
try:
if isinstance(obj, dict):
return obj
return asdict(obj) if is_dataclass(obj) else dict(obj)
except Exception:
return {}

@staticmethod
def validate_kwargs(
Expand All @@ -227,7 +235,7 @@ def validate_kwargs(
sig = signature(func)
fields = {
n: (
p.annotation,
Any if p.annotation is Parameter.empty else p.annotation,
... if p.default is Parameter.empty else p.default,
)
for n, p in sig.parameters.items()
Expand Down
21 changes: 11 additions & 10 deletions openbb_platform/core/openbb_core/app/static/package_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,10 +528,12 @@ def get_deprecation_message(path: str) -> str:
return getattr(PathHandler.build_route_map()[path], "summary", "")

@staticmethod
def reorder_params(params: Dict[str, Parameter]) -> "OrderedDict[str, Parameter]":
"""Reorder the params."""
def reorder_params(
params: Dict[str, Parameter], var_kw: Optional[List[str]] = None
) -> "OrderedDict[str, Parameter]":
"""Reorder the params and make sure VAR_KEYWORD come after 'provider."""
formatted_keys = list(params.keys())
for k in ["provider", "extra_params"]:
for k in ["provider"] + (var_kw or []):
if k in formatted_keys:
formatted_keys.remove(k)
formatted_keys.append(k)
Expand Down Expand Up @@ -563,14 +565,11 @@ def format_params(
)

formatted: Dict[str, Parameter] = {}

var_kw = []
for name, param in parameter_map.items():
if name == "extra_params":
formatted[name] = Parameter(name="kwargs", kind=Parameter.VAR_KEYWORD)
elif name == "kwargs":
formatted["**" + name] = Parameter(
name="kwargs", kind=Parameter.VAR_KEYWORD, annotation=Any
)
var_kw.append(name)
elif name == "provider_choices":
fields = param.annotation.__args__[0].__dataclass_fields__
field = fields["provider"]
Expand Down Expand Up @@ -624,12 +623,14 @@ def format_params(

formatted[name] = Parameter(
name=name,
kind=Parameter.POSITIONAL_OR_KEYWORD,
kind=param.kind,
annotation=updated_type,
default=param.default,
)
if param.kind == Parameter.VAR_KEYWORD:
var_kw.append(name)

return MethodDefinition.reorder_params(params=formatted)
return MethodDefinition.reorder_params(params=formatted, var_kw=var_kw)

@staticmethod
def add_field_custom_annotations(
Expand Down
60 changes: 50 additions & 10 deletions openbb_platform/core/tests/app/static/test_package_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,17 +206,57 @@ class TestAnnotatedDataClass:
assert result


def test_reorder_params(method_definition):
"""Test reorder params."""
params = {
"provider": Parameter.empty,
"extra_params": Parameter.empty,
"param1": Parameter.empty,
"param2": Parameter.empty,
}
result = method_definition.reorder_params(params=params)
@pytest.mark.parametrize(
"params, var_kw, expected",
[
(
{
"provider": Parameter.empty,
"extra_params": Parameter.empty,
"param1": Parameter.empty,
"param2": Parameter.empty,
},
None,
["extra_params", "param1", "param2", "provider"],
),
(
{
"param1": Parameter.empty,
"provider": Parameter.empty,
"extra_params": Parameter.empty,
"param2": Parameter.empty,
},
["extra_params"],
["param1", "param2", "provider", "extra_params"],
),
(
{
"param2": Parameter.empty,
"any_kwargs": Parameter.empty,
"provider": Parameter.empty,
"param1": Parameter.empty,
},
["any_kwargs"],
["param2", "param1", "provider", "any_kwargs"],
),
(
{
"any_kwargs": Parameter.empty,
"extra_params": Parameter.empty,
"provider": Parameter.empty,
"param1": Parameter.empty,
"param2": Parameter.empty,
},
["any_kwargs", "extra_params"],
["param1", "param2", "provider", "any_kwargs", "extra_params"],
),
],
)
def test_reorder_params(method_definition, params, var_kw, expected):
"""Test reorder params, ensure var_kw are last after 'provider'."""
result = method_definition.reorder_params(params, var_kw)
assert result
assert list(result.keys()) == ["param1", "param2", "provider", "extra_params"]
assert list(result.keys()) == expected


def test_build_func_params(method_definition):
Expand Down
19 changes: 16 additions & 3 deletions openbb_platform/core/tests/app/test_command_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from openbb_core.app.model.command_context import CommandContext
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 ExtraParams
from openbb_core.app.router import CommandMap
from pydantic import BaseModel, ConfigDict

Expand Down Expand Up @@ -228,45 +229,57 @@ def test_parameters_builder_validate_kwargs(mock_func):


@pytest.mark.parametrize(
"provider_choices, extra_params, expect",
"provider_choices, extra_params, base, expect",
[
(
{"provider": "provider1"},
{"exists_in_2": ...},
ExtraParams,
OpenBBWarning,
),
(
{"provider": "inexistent_provider"},
{"exists_in_both": ...},
ExtraParams,
OpenBBWarning,
),
(
{},
{"inexistent_field": ...},
ExtraParams,
OpenBBWarning,
),
(
{},
{"inexistent_field": ...},
object,
None,
),
(
{"provider": "provider2"},
{"exists_in_2": ...},
ExtraParams,
None,
),
(
{"provider": "provider2"},
{"exists_in_both": ...},
ExtraParams,
None,
),
(
{},
{"exists_in_both": ...},
ExtraParams,
None,
),
],
)
def test_parameters_builder__warn_kwargs(provider_choices, extra_params, expect):
def test_parameters_builder__warn_kwargs(provider_choices, extra_params, base, expect):
"""Test _warn_kwargs."""

@dataclass
class SomeModel:
class SomeModel(base):
"""SomeModel"""

exists_in_2: QueryParam = Query(..., title="provider2")
Expand Down
Loading