Skip to content

fix kwonlydefaults key error in filter_args#715

Merged
GaelVaroquaux merged 1 commit intojoblib:masterfrom
noamher:master
Jul 11, 2018
Merged

fix kwonlydefaults key error in filter_args#715
GaelVaroquaux merged 1 commit intojoblib:masterfrom
noamher:master

Conversation

@noamher
Copy link
Copy Markdown
Contributor

@noamher noamher commented Jul 8, 2018

Fix for key error which were thrown from filter_args when default values were not specified for kw only arguments.

For example, in the following case arg_spec.kwonlydefaults is None:

def foo(x, *, y):
    pass

filter_args(foo, [], [1], dict(y=2))

.../env/lib/python3.5/site-packages/sklearn/externals/joblib/func_inspect.py in <genexpr>(.0)
    242     arg_defaults = arg_spec.defaults or ()
    243     arg_defaults = arg_defaults + tuple(arg_spec.kwonlydefaults[k]
--> 244                                         for k in arg_spec.kwonlyargs)
    245     arg_varargs = arg_spec.varargs
    246     arg_varkw = arg_spec.varkw

TypeError: 'NoneType' object is not subscriptable

In another example, arg_spec.kwonlydefaults is not None, but it does not contain 'y':

def foo(x, *, y, z=3):
    pass

filter_args(foo, [], [1], dict(y=2))

.../env/lib/python3.5/site-packages/sklearn/externals/joblib/func_inspect.py in <genexpr>(.0)
    242     arg_defaults = arg_spec.defaults or ()
    243     arg_defaults = arg_defaults + tuple(arg_spec.kwonlydefaults[k]
--> 244                                         for k in arg_spec.kwonlyargs)
    245     arg_varargs = arg_spec.varargs
    246     arg_varkw = arg_spec.varkw

KeyError: 'y'

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Jul 8, 2018

Looks overall good, but the tests now are incompatible with python 2, and we still support python 2.

@noamher
Copy link
Copy Markdown
Contributor Author

noamher commented Jul 10, 2018

would you like me to remove the changes to the tests and keep only the changes to filter_args?

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Jul 10, 2018 via email

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 10, 2018 •
edited
Loading

Codecov Report

Merging #715 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
- Coverage   95.07%   95.03%   -0.05%     
==========================================
  Files          40       40              
  Lines        5751     5758       +7     
==========================================
+ Hits         5468     5472       +4     
- Misses        283      286       +3
Impacted Files Coverage Δ
joblib/test/test_func_inspect.py 91.59% <100%> (+0.44%) ⬆️
joblib/func_inspect.py 94.88% <100%> (+0.02%) ⬆️
joblib/_parallel_backends.py 95.6% <0%> (-1.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a2f906...ce0e808. Read the comment docs.

@noamher noamher force-pushed the master branch 3 times, most recently from 3f214e6 to 7e36897 Compare July 10, 2018 14:52
@noamher
Copy link
Copy Markdown
Contributor Author

noamher commented Jul 11, 2018

@GaelVaroquaux do you know why it fails the CircleCI test?

I changed the extra test to run only under python 3.

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Jul 11, 2018 via email

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Jul 11, 2018

Circle ran without problem. I am merging.

@GaelVaroquaux GaelVaroquaux merged commit 821b316 into joblib:master Jul 11, 2018
@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Jul 11, 2018

Thank you!!

@noamher
Copy link
Copy Markdown
Contributor Author

noamher commented Jul 11, 2018

👍

yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Jul 28, 2018
@yarikoptic
Merge tag '0.12.1' into releases
22dd692 
* tag '0.12.1':
  Release 0.12.1
  fix kwonlydefaults key error in filter_args (joblib#715)
  MNT fix some "undefined name" flake8 warnings (joblib#713).
  from importlib import reload for Python 3 (joblib#675)
  MTN vendor loky2.1.4 (joblib#708)
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Jul 28, 2018
@yarikoptic
Merge branch 'releases' into debian
c9a3fb2 
* releases: (121 commits)
  Release 0.12.1
  fix kwonlydefaults key error in filter_args (joblib#715)
  MNT fix some "undefined name" flake8 warnings (joblib#713).
  from importlib import reload for Python 3 (joblib#675)
  MTN vendor loky2.1.4 (joblib#708)
  Release 0.12
  typo
  typo
  typo
  ENH add initializer limiting n_threads for C-libs (joblib#701)
  DOC better parallel docstring (joblib#704)
  [MRG] Nested parallel call thread bomb mitigation (joblib#700)
  MTN vendor loky2.1.3 (joblib#699)
  Make it possible to configure the reusable executor workers timeout (joblib#698)
  MAINT increase timeouts to make test more robust on travis
  DOC: use the .joblib extension instead of .pkl (joblib#697)
  [MRG] Fix exception handling in nested parallel calls (joblib#696)
  Fix skip test lz4 not installed (joblib#695)
  [MRG] numpy_pickle:  several enhancements (joblib#626)
  Introduce Parallel.__call__ backend callbacks (joblib#689)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@noamher @GaelVaroquaux