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

@check_types now properly passes in *args **kwargs and checks their types #1336

Merged
merged 6 commits into from
Jan 25, 2024

Conversation

ecthompson99
Copy link
Contributor

@ecthompson99 ecthompson99 commented Sep 10, 2023

Background

  • Unexpected behavior of functions with *args or **kwargs when @check_types decorator is applied
  • Root cause:
    • validate_inputs() uses sig.bind_partial(*args).arguments which returns an OrderedDict of arguments or keyword arguments. However:
      • *args are bundled into a single value: {arg1: 1, args: (2,3,4)} which is then returned to the wrapped function as (1, (2,3,4)) instead of (1,2,3,4)
      • **kwargs are bundled into a nested dictionary: {kwarg1: 1, kwargs: {kwarg2: 2, kwarg3: 3}} which is then returned to the wrapped function as {kwarg1: 1, {kwargs: {kwarg2: 2, kwarg3: 3}} instead of {kwarg1: 1, kwarg2: 2, kwarg3: 3}

Implementation

All changes added to def check_types (here)

  • Additional logic is needed to handle cases when *args or **kwargs are passed in
  • Since args and kwargs are treated differently, I split the validate_args into two validation functions -- one for positional arguments, one for keyword arguments
    • def validate_args:
      • Check if there are *args in function arguments by comparing the length of the named arguments (from sig.bind_partial(*args).arguments) and the nominal arguments. If the lengths match, no *args, and handle normally
      • If there are *args, handle the explicit args normally, and then validate each argument in *args, iteratively
      • Combine explicit and star_args into single list to pass into the function
    • def validate_kwargs:
      • Check if there are **kwargs in function arguments by comparing the keys in sig.bind_partial(**kwargs).arguments to the keys in the nominal keyword args. If they are different, then **kwargs exist
      • If there are **kwargs, handle explicit kwargs normally, and then validate each kwarg in **kwargs, iteratively
      • Combine explicit and star_kwargs into single list to pass into the function

Related Issue

#1334

Testing

  • Added 3 tests:
    1. test_check_types_star_args: Test for *args by comparing argument lengths
    2. test_check_types_star_kwargs: Test for **kwargs by comparing keys
    3. test_check_types_star_args_kwargs: Test that the values for both *args and **kwargs get passed through correctly

@cosmicBboy
Copy link
Collaborator

thanks for the PR! looks like there are some mypy lint errors to address

@ecthompson99
Copy link
Contributor Author

thanks for the PR! looks like there are some mypy lint errors to address

Hi @cosmicBboy, addressed the mypy and black formatting issues in the most recent commit, thanks!

@cosmicBboy cosmicBboy closed this Jan 25, 2024
@cosmicBboy cosmicBboy reopened this Jan 25, 2024
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ceeae10) 93.91% compared to head (6b1396e) 94.29%.
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1336      +/-   ##
==========================================
+ Coverage   93.91%   94.29%   +0.37%     
==========================================
  Files          91       91              
  Lines        6775     7024     +249     
==========================================
+ Hits         6363     6623     +260     
+ Misses        412      401      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cosmicBboy
Copy link
Collaborator

Looks good @ecthompson99 merging now. Congrats on your first PR to pandera!

@cosmicBboy cosmicBboy merged commit 4df61da into unionai-oss:main Jan 25, 2024
56 checks passed
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

Successfully merging this pull request may close these issues.

2 participants