Repository: joblib/joblib

Issue:
Replace deprecated usage of inspect.getargspec
inspect.getargspec has been deprecated since 3.0 (in favor of inspect.getfullargspec until 3.2 and in favor of inspect.signature since 3.3) and will be removed in 3.6. It also creates visible DeprecationWarning in 3.5.


Discussion:
lesteve: > inspect.getfullargspec is marked as deprecated since 3.5, why not directly use a compatibility function called signature and use the one from python >= 3.4.

Yeah I am aware of that but I wanted to do the smallest change to make joblib.Memory support functions with signature or keyword-only arguments. scikit-learn went the way you mentioned and ended up having to backport OrderedDict in order to support python 2.6.
aabadie: @lesteve, to me your change is an improvement and I'm ok to merge it. But if you have time and think it's worth reusing the scikit-learn strategy, then go ahead ;)
lesteve: > @lesteve, to me your change is an improvement and I'm ok to merge it. But if you have time and think it's worth reusing the scikit-learn strategy, then go ahead ;)

I am enclined to go with this simpler strategy for now. The only thing I want to look at is tackling this comment from above:

> At the moment I am not doing any checks to see whether the keyword-only arguments were indeed passed as keywords. Maybe I should since there is some checks for the number of arguments in this function already.
lesteve: Right, I fixed the keyword-only argument passed as positional parameter in joblib.function_inspect.filter_args. As you would expect, the snippet above now raises

```
ValueError: Keyword-only parameter 'kw1' was passed as positional parameter for func_with_kwonly_args(a, b, *, kw1='kw1', kw2='kw2'):
     func_with_kwonly_args(1, 2, 3, kw2=4) was called.
```

Any more comments ?
aabadie: @lesteve, @ogrisel, could this one be merged ?
ogrisel: Besides minor comments, LGTM. I can do the cosmetics changed when merging if you wish.

Patch:
diff --git a/joblib/func_inspect.py b/joblib/func_inspect.py
index cc5cbf6..3eee40b 100644
--- a/joblib/func_inspect.py
+++ b/joblib/func_inspect.py
@@ -11,11 +11,15 @@ import inspect
 import warnings
 import re
 import os
-
+import sys
 from ._compat import _basestring
 from .logger import pformat
 from ._memory_helpers import open_py_source
 
+
+PY3 = sys.version_info[0] >= 3
+
+
 def get_func_code(func):
     """ Attempts to retrieve a reliable function code hash.
 
@@ -156,6 +160,53 @@ def get_func_name(func, resolv_alias=True, win_characters=True):
     return module, name
 
 
+def getfullargspec(func):
+    """Compatibility function to provide inspect.getfullargspec in Python 2
+
+    This should be rewritten using a backport of Python 3 signature
+    once we drop support for Python 2.6. We went for a simpler
+    approach at the time of writing because signature uses OrderedDict
+    which is not available in Python 2.6.
+    """
+    try:
+        return inspect.getfullargspec(func)
+    except AttributeError:
+        arg_spec = inspect.getargspec(func)
+        import collections
+        tuple_fields = ('args varargs varkw defaults kwonlyargs '
+                        'kwonlydefaults annotations')
+        tuple_type = collections.namedtuple('FullArgSpec', tuple_fields)
+
+        return tuple_type(args=arg_spec.args,
+                          varargs=arg_spec.varargs,
+                          varkw=arg_spec.keywords,
+                          defaults=arg_spec.defaults,
+                          kwonlyargs=[],
+                          kwonlydefaults=None,
+                          annotations={})
+
+
+def _signature_str(function_name, arg_spec):
+    """Helper function to output a function signature"""
+    # inspect.formatargspec can not deal with the same
+    # number of arguments in python 2 and 3
+    arg_spec_for_format = arg_spec[:7 if PY3 else 4]
+
+    arg_spec_str = inspect.formatargspec(*arg_spec_for_format)
+    return '{0}{1}'.format(function_name, arg_spec_str)
+
+
+def _function_called_str(function_name, args, kwargs):
+    """Helper function to output a function call"""
+    template_str = '{0}({1}, {2})'
+
+    args_str = repr(args)[1:-1]
+    kwargs_str = ', '.join('%s=%s' % (k, v)
+                           for k, v in kwargs.items())
+    return template_str.format(function_name, args_str,
+                               kwargs_str)
+
+
 def filter_args(func, ignore_lst, args=(), kwargs=dict()):
     """ Filters the given args and kwargs using a list of arguments to
         ignore, and a function specification.
@@ -180,19 +231,22 @@ def filter_args(func, ignore_lst, args=(), kwargs=dict()):
     args = list(args)
     if isinstance(ignore_lst, _basestring):
         # Catch a common mistake
-        raise ValueError('ignore_lst must be a list of parameters to ignore '
+        raise ValueError(
+            'ignore_lst must be a list of parameters to ignore '
             '%s (type %s) was given' % (ignore_lst, type(ignore_lst)))
     # Special case for functools.partial objects
     if (not inspect.ismethod(func) and not inspect.isfunction(func)):
         if ignore_lst:
             warnings.warn('Cannot inspect object %s, ignore list will '
-                'not work.' % func, stacklevel=2)
+                          'not work.' % func, stacklevel=2)
         return {'*': args, '**': kwargs}
-    arg_spec = inspect.getargspec(func)
-    arg_names = arg_spec.args
-    arg_defaults = arg_spec.defaults or {}
-    arg_keywords = arg_spec.keywords
+    arg_spec = getfullargspec(func)
+    arg_names = arg_spec.args + arg_spec.kwonlyargs
+    arg_defaults = arg_spec.defaults or ()
+    arg_defaults = arg_defaults + tuple(arg_spec.kwonlydefaults[k]
+                                        for k in arg_spec.kwonlyargs)
     arg_varargs = arg_spec.varargs
+    arg_varkw = arg_spec.varkw
 
     if inspect.ismethod(func):
         # First argument is 'self', it has been removed by Python
@@ -207,7 +261,18 @@ def filter_args(func, ignore_lst, args=(), kwargs=dict()):
     for arg_position, arg_name in enumerate(arg_names):
         if arg_position < len(args):
             # Positional argument or keyword argument given as positional
-            arg_dict[arg_name] = args[arg_position]
+            if arg_name not in arg_spec.kwonlyargs:
+                arg_dict[arg_name] = args[arg_position]
+            else:
+                raise ValueError(
+                    "Keyword-only parameter '%s' was passed as "
+                    'positional parameter for %s:\n'
+                    '     %s was called.'
+                    % (arg_name,
+                       _signature_str(name, arg_spec),
+                       _function_called_str(name, args, kwargs))
+                )
+
         else:
             position = arg_position - len(arg_names)
             if arg_name in kwargs:
@@ -217,28 +282,24 @@ def filter_args(func, ignore_lst, args=(), kwargs=dict()):
                     arg_dict[arg_name] = arg_defaults[position]
                 except (IndexError, KeyError):
                     # Missing argument
-                    raise ValueError('Wrong number of arguments for %s%s:\n'
-                                     '     %s(%s, %s) was called.'
-                        % (name,
-                           inspect.formatargspec(*inspect.getargspec(func)),
-                           name,
-                           repr(args)[1:-1],
-                           ', '.join('%s=%s' % (k, v)
-                                    for k, v in kwargs.items())
-                           )
-                        )
+                    raise ValueError(
+                        'Wrong number of arguments for %s:\n'
+                        '     %s was called.'
+                        % (_signature_str(name, arg_spec),
+                           _function_called_str(name, args, kwargs))
+                    )
 
     varkwargs = dict()
     for arg_name, arg_value in sorted(kwargs.items()):
         if arg_name in arg_dict:
             arg_dict[arg_name] = arg_value
-        elif arg_keywords is not None:
+        elif arg_varkw is not None:
             varkwargs[arg_name] = arg_value
         else:
             raise TypeError("Ignore list for %s() contains an unexpected "
                             "keyword argument '%s'" % (name, arg_name))
 
-    if arg_keywords is not None:
+    if arg_varkw is not None:
         arg_dict['**'] = varkwargs
     if arg_varargs is not None:
         varargs = args[arg_position + 1:]
@@ -250,13 +311,10 @@ def filter_args(func, ignore_lst, args=(), kwargs=dict()):
             arg_dict.pop(item)
         else:
             raise ValueError("Ignore list: argument '%s' is not defined for "
-            "function %s%s" %
-                            (item, name,
-                             inspect.formatargspec(arg_names,
-                                                   arg_varargs,
-                                                   arg_keywords,
-                                                   arg_defaults,
-                                                   )))
+                             "function %s"
+                             % (item,
+                                _signature_str(name, arg_spec))
+            )
     # XXX: Return a sorted list of pairs?
     return arg_dict
 


Test patch:
diff --git a/joblib/test/test_func_inspect.py b/joblib/test/test_func_inspect.py
index 15c6b43..62920e9 100644
--- a/joblib/test/test_func_inspect.py
+++ b/joblib/test/test_func_inspect.py
@@ -11,12 +11,15 @@ import shutil
 import nose
 import tempfile
 import functools
+import sys
 
 from joblib.func_inspect import filter_args, get_func_name, get_func_code
 from joblib.func_inspect import _clean_win_chars, format_signature
 from joblib.memory import Memory
 from joblib.test.common import with_numpy
+from joblib.testing import assert_raises_regex
 
+PY3 = sys.version_info[0] >= 3
 
 ###############################################################################
 # Module-level functions, for tests
@@ -165,6 +168,37 @@ def test_func_inspect_errors():
                                     __file__.replace('.pyc', '.py'))
 
 
+if PY3:
+    exec("""
+def func_with_kwonly_args(a, b, *, kw1='kw1', kw2='kw2'): pass
+
+def func_with_signature(a: int, b: int) -> None: pass
+""")
+
+    def test_filter_args_python_3():
+        nose.tools.assert_equal(
+            filter_args(func_with_kwonly_args,
+                        [], (1, 2), {'kw1': 3, 'kw2': 4}),
+            {'a': 1, 'b': 2, 'kw1': 3, 'kw2': 4})
+
+        # filter_args doesn't care about keyword-only arguments so you
+        # can pass 'kw1' into *args without any problem
+        assert_raises_regex(
+            ValueError,
+            "Keyword-only parameter 'kw1' was passed as positional parameter",
+            filter_args,
+            func_with_kwonly_args, [], (1, 2, 3), {'kw2': 2})
+
+        nose.tools.assert_equal(
+            filter_args(func_with_kwonly_args, ['b', 'kw2'], (1, 2),
+                        {'kw1': 3, 'kw2': 4}),
+            {'a': 1, 'kw1': 3})
+
+        nose.tools.assert_equal(
+            filter_args(func_with_signature, ['b'], (1, 2)),
+            {'a': 1})
+
+
 def test_bound_methods():
     """ Make sure that calling the same method on two different instances
         of the same class does resolv to different signatures.
diff --git a/joblib/test/test_memory.py b/joblib/test/test_memory.py
index 14e319f..5f0bbfd 100644
--- a/joblib/test/test_memory.py
+++ b/joblib/test/test_memory.py
@@ -21,7 +21,9 @@ import nose
 from joblib.memory import Memory, MemorizedFunc, NotMemorizedFunc, MemorizedResult
 from joblib.memory import NotMemorizedResult, _FUNCTION_HASHES
 from joblib.test.common import with_numpy, np
+from joblib.testing import assert_raises_regex
 
+PY3 = sys.version_info[0] >= 3
 
 ###############################################################################
 # Module-level variables for the tests
@@ -676,3 +678,48 @@ def test_memory_in_memory_function_code_change():
 def test_clear_memory_with_none_cachedir():
     mem = Memory(cachedir=None)
     mem.clear()
+
+if PY3:
+    exec("""
+def func_with_kwonly_args(a, b, *, kw1='kw1', kw2='kw2'):
+    return a, b, kw1, kw2
+
+def func_with_signature(a: int, b: float) -> float:
+    return a + b
+""")
+
+    def test_memory_func_with_kwonly_args():
+        mem = Memory(cachedir=env['dir'], verbose=0)
+        func_cached = mem.cache(func_with_kwonly_args)
+
+        nose.tools.assert_equal(func_cached(1, 2, kw1=3), (1, 2, 3, 'kw2'))
+
+        # Making sure that providing a keyword-only argument by
+        # position raises an exception
+        assert_raises_regex(
+            ValueError,
+            "Keyword-only parameter 'kw1' was passed as positional parameter",
+            func_cached,
+            1, 2, 3, {'kw2': 4})
+
+        # Keyword-only parameter passed by position with cached call
+        # should still raise ValueError
+        func_cached(1, 2, kw1=3, kw2=4)
+
+        assert_raises_regex(
+            ValueError,
+            "Keyword-only parameter 'kw1' was passed as positional parameter",
+            func_cached,
+            1, 2, 3, {'kw2': 4})
+
+        # Test 'ignore' parameter
+        func_cached = mem.cache(func_with_kwonly_args, ignore=['kw2'])
+        nose.tools.assert_equal(func_cached(1, 2, kw1=3, kw2=4), (1, 2, 3, 4))
+        nose.tools.assert_equal(func_cached(1, 2, kw1=3, kw2='ignored'), (1, 2, 3, 4))
+
+
+    def test_memory_func_with_signature():
+        mem = Memory(cachedir=env['dir'], verbose=0)
+        func_cached = mem.cache(func_with_signature)
+
+        nose.tools.assert_equal(func_cached(1, 2.), 3.)