Repository: joblib/joblib

Issue:
Caching breaks when new (uncached) args are treated as kwargs.
Simple fix addressed with PR: https://github.com/joblib/joblib/pull/750

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/joblib/joblib/pull/750?src=pr&el=h1) Report
> Merging [#750](https://codecov.io/gh/joblib/joblib/pull/750?src=pr&el=desc) into [master](https://codecov.io/gh/joblib/joblib/commit/b0bc5dd24b123b445965a5f4d487beaf6fadd083?src=pr&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/joblib/joblib/pull/750/graphs/tree.svg?width=650&height=150&src=pr&token=gA6LF5DGTW)](https://codecov.io/gh/joblib/joblib/pull/750?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
+ Coverage   95.24%   95.29%   +0.04%     
==========================================
  Files          43       43              
  Lines        6137     6137              
==========================================
+ Hits         5845     5848       +3     
+ Misses        292      289       -3
```


| [Impacted Files](https://codecov.io/gh/joblib/joblib/pull/750?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [joblib/func\_inspect.py](https://codecov.io/gh/joblib/joblib/pull/750/diff?src=pr&el=tree#diff-am9ibGliL2Z1bmNfaW5zcGVjdC5weQ==) | `95.45% <100%> (+0.56%)` | :arrow_up: |
| [joblib/\_parallel\_backends.py](https://codecov.io/gh/joblib/joblib/pull/750/diff?src=pr&el=tree#diff-am9ibGliL19wYXJhbGxlbF9iYWNrZW5kcy5weQ==) | `97.2% <0%> (-1.21%)` | :arrow_down: |
| [joblib/test/test\_parallel.py](https://codecov.io/gh/joblib/joblib/pull/750/diff?src=pr&el=tree#diff-am9ibGliL3Rlc3QvdGVzdF9wYXJhbGxlbC5weQ==) | `96.73% <0%> (+0.14%)` | :arrow_up: |
| [joblib/\_store\_backends.py](https://codecov.io/gh/joblib/joblib/pull/750/diff?src=pr&el=tree#diff-am9ibGliL19zdG9yZV9iYWNrZW5kcy5weQ==) | `90.47% <0%> (+0.52%)` | :arrow_up: |
| [joblib/pool.py](https://codecov.io/gh/joblib/joblib/pull/750/diff?src=pr&el=tree#diff-am9ibGliL3Bvb2wucHk=) | `91.37% <0%> (+1.72%)` | :arrow_up: |
| [joblib/backports.py](https://codecov.io/gh/joblib/joblib/pull/750/diff?src=pr&el=tree#diff-am9ibGliL2JhY2twb3J0cy5weQ==) | `95.83% <0%> (+2.08%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/joblib/joblib/pull/750?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/joblib/joblib/pull/750?src=pr&el=footer). Last update [b0bc5dd...a2418f1](https://codecov.io/gh/joblib/joblib/pull/750?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lesteve: Good point, I ran a git bisect and it is another regression in 0.12 following the `Memory` store backend refactoring in #397. Would you be kind enough to add a test in `joblib/test/test_memory.py` and maybe one in `joblib/test/test_func_inspect.py` (to make sure that `filter_args` does not have a side effect on its `kwargs` parameter?

I have to say I don't really understand why filter_args was modifying its `kwargs` parameter but this may require a closer look at the changes that went into #397.
collijk: Yep.  I'll take a look later today at the tests.  
collijk: Now with regression tests against the end-to-end `Memory` usage and one particular use case of `filter_args`.  

pytest output before fix:
```
======================================================================================= test session starts ========================================================================================
platform linux -- Python 3.6.6, pytest-3.7.2, py-1.5.4, pluggy-0.7.1
rootdir: /home/james/code/external_tools/joblib, inifile: setup.cfg
collected 1070 items / 2 skipped                                                                                                                                                                   

joblib/__init__.py s                                                                                                                                                                         [  0%]
joblib/parallel.py ss                                                                                                                                                                        [  0%]
joblib/test/test_backports.py s....                                                                                                                                                          [  0%]
joblib/test/test_disk.py .........                                                                                                                                                           [  1%]
joblib/test/test_format_stack.py s...                                                                                                                                                        [  1%]
joblib/test/test_func_inspect.py s.............................F......                                                                                                                       [  5%]
joblib/test/test_hashing.py sssssssssss..................................................................................................................................................... [ 20%]
............................................................................................................................................................................................ [ 37%]
....................................................................................................................................................ss.............                          [ 53%]
joblib/test/test_logger.py .                                                                                                                                                                 [ 53%]
joblib/test/test_memmapping.py ssssssssssssss..                                                                                                                                              [ 54%]
joblib/test/test_memory.py ss............F............................                                                                                                                       [ 58%]
joblib/test/test_my_exceptions.py ...                                                                                                                                                        [ 59%]
joblib/test/test_numpy_pickle.py sssssssssssssssssssss................................................................................s.                                                     [ 68%]
joblib/test/test_numpy_pickle_compat.py .                                                                                                                                                    [ 68%]
joblib/test/test_numpy_pickle_utils.py ..                                                                                                                                                    [ 68%]
joblib/test/test_parallel.py sssss.......................................................................................................................................................... [ 83%]
..................................................................................................................................................................ss                         [ 99%]
joblib/test/test_store_backends.py ...                                                                                                                                                       [ 99%]
joblib/test/test_testing.py .....                                                                                                                                                            [ 99%]
joblib/test/data/create_numpy_pickle.py s                                                                                                                                                    [100%]

============================================================================================= FAILURES =============================================================================================
_______________________________________________________________________________ test_filter_args_kwargs_consumption ________________________________________________________________________________

    def test_filter_args_kwargs_consumption():
        """Regression test against 0.12.0 changes.
    
        Make sure filter args doesn't consume the kwargs dict that gets passed to it.
        """
        kwargs = {'x': 0}
        filter_args(g, [], [], kwargs)
>       assert kwargs == {'x': 0}
E       AssertionError: assert {} == {'x': 0}
E         Right contains more items:
E         {'x': 0}
E         Use -v to get the full diff

joblib/test/test_func_inspect.py:233: AssertionError
____________________________________________________________________________________ test_memory_args_as_kwargs ____________________________________________________________________________________

func = <function test_memory_args_as_kwargs.<locals>.plus_one at 0x7f0d9fdd67b8>, ignore_lst = [], args = [], kwargs = {}

    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.
    
            Parameters
            ----------
            func: callable
                Function giving the argument specification
            ignore_lst: list of strings
                List of arguments to ignore (either a name of an argument
                in the function spec, or '*', or '**')
            *args: list
                Positional arguments passed to the function.
            **kwargs: dict
                Keyword arguments passed to the function
    
            Returns
            -------
            filtered_args: list
                List of filtered positional and keyword arguments.
        """
        args = list(args)
        if isinstance(ignore_lst, _basestring):
            # Catch a common mistake
            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)
            return {'*': args, '**': kwargs}
        arg_spec = getfullargspec(func)
        arg_names = arg_spec.args + arg_spec.kwonlyargs
        arg_defaults = arg_spec.defaults or ()
        if arg_spec.kwonlydefaults:
            arg_defaults = arg_defaults + tuple(arg_spec.kwonlydefaults[k]
                                                for k in arg_spec.kwonlyargs
                                                if k in arg_spec.kwonlydefaults)
        arg_varargs = arg_spec.varargs
        arg_varkw = arg_spec.varkw
    
        if inspect.ismethod(func):
            # First argument is 'self', it has been removed by Python
            # we need to add it back:
            args = [func.__self__, ] + args
        # XXX: Maybe I need an inspect.isbuiltin to detect C-level methods, such
        # as on ndarrays.
    
        _, name = get_func_name(func, resolv_alias=False)
        arg_dict = dict()
        arg_position = -1
        for arg_position, arg_name in enumerate(arg_names):
            if arg_position < len(args):
                # Positional argument or keyword argument given as positional
                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:
                    arg_dict[arg_name] = kwargs.pop(arg_name)
                else:
                    try:
>                       arg_dict[arg_name] = arg_defaults[position]
E                       IndexError: tuple index out of range

joblib/func_inspect.py:281: IndexError

During handling of the above exception, another exception occurred:

tmpdir = local('/tmp/pytest-of-james/pytest-13/test_memory_args_as_kwargs0')

    def test_memory_args_as_kwargs(tmpdir):
        """Regression test against 0.12.0 changes."""
        memory = Memory(location=tmpdir.strpath, verbose=0)
    
        @memory.cache
        def plus_one(a):
            return a + 1
    
        # This would (and should) pass before the patch.
        assert plus_one(1) == 2
        assert plus_one(a=1) == 2
    
        # However, a positional argument that joblib hadn't seen
        # before would cause a failure if it was passed as a kwarg
>       assert plus_one(a=2) == 3

joblib/test/test_memory.py:362: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
joblib/memory.py:514: in __call__
    return self._cached_call(args, kwargs)[0]
joblib/memory.py:458: in _cached_call
    out, metadata = self.call(*args, **kwargs)
joblib/memory.py:685: in call
    metadata = self._persist_input(duration, args, kwargs)
joblib/memory.py:719: in _persist_input
    func_id, args_id = self._get_output_identifiers(*args, **kwargs)
joblib/memory.py:535: in _get_output_identifiers
    argument_hash = self._get_argument_hash(*args, **kwargs)
joblib/memory.py:529: in _get_argument_hash
    return hashing.hash(filter_args(self.func, self.ignore, args, kwargs),
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

func = <function test_memory_args_as_kwargs.<locals>.plus_one at 0x7f0d9fdd67b8>, ignore_lst = [], args = [], kwargs = {}

    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.
    
            Parameters
            ----------
            func: callable
                Function giving the argument specification
            ignore_lst: list of strings
                List of arguments to ignore (either a name of an argument
                in the function spec, or '*', or '**')
            *args: list
                Positional arguments passed to the function.
            **kwargs: dict
                Keyword arguments passed to the function
    
            Returns
            -------
            filtered_args: list
                List of filtered positional and keyword arguments.
        """
        args = list(args)
        if isinstance(ignore_lst, _basestring):
            # Catch a common mistake
            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)
            return {'*': args, '**': kwargs}
        arg_spec = getfullargspec(func)
        arg_names = arg_spec.args + arg_spec.kwonlyargs
        arg_defaults = arg_spec.defaults or ()
        if arg_spec.kwonlydefaults:
            arg_defaults = arg_defaults + tuple(arg_spec.kwonlydefaults[k]
                                                for k in arg_spec.kwonlyargs
                                                if k in arg_spec.kwonlydefaults)
        arg_varargs = arg_spec.varargs
        arg_varkw = arg_spec.varkw
    
        if inspect.ismethod(func):
            # First argument is 'self', it has been removed by Python
            # we need to add it back:
            args = [func.__self__, ] + args
        # XXX: Maybe I need an inspect.isbuiltin to detect C-level methods, such
        # as on ndarrays.
    
        _, name = get_func_name(func, resolv_alias=False)
        arg_dict = dict()
        arg_position = -1
        for arg_position, arg_name in enumerate(arg_names):
            if arg_position < len(args):
                # Positional argument or keyword argument given as positional
                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:
                    arg_dict[arg_name] = kwargs.pop(arg_name)
                else:
                    try:
                        arg_dict[arg_name] = arg_defaults[position]
                    except (IndexError, KeyError):
                        # Missing argument
                        raise ValueError(
                            'Wrong number of arguments for %s:\n'
                            '     %s was called.'
                            % (_signature_str(name, arg_spec),
>                              _function_called_str(name, args, kwargs))
                        )
E                       ValueError: Wrong number of arguments for plus_one(a):
E                            plus_one(, ) was called.

joblib/func_inspect.py:288: ValueError
======================================================================== 2 failed, 1003 passed, 67 skipped in 54.76 seconds ========================================================================
[INFO:MainProcess:MainThread] process shutting down
[DEBUG:MainProcess:MainThread] running all "atexit" finalizers with priority >= 0
[DEBUG:MainProcess:MainThread] Interpreter shutting down. Waking up queue_manager_threads [(<Thread(QueueManagerThread, started daemon 139695514167040)>, <joblib.externals.loky.process_executor._ThreadWakeup object at 0x7f0d9cf0c6a0>)]
[DEBUG:MainProcess:QueueManagerThread] queue management thread shutting down
[DEBUG:MainProcess:QueueManagerThread] closing call_queue
[DEBUG:MainProcess:QueueManagerThread] telling queue thread to quit
[DEBUG:MainProcess:QueueManagerThread] joining processes
[DEBUG:MainProcess:QueueFeederThread] feeder thread got sentinel -- exiting
[DEBUG:MainProcess:QueueManagerThread] queue management thread clean shutdown of worker processes: []
[DEBUG:MainProcess:MainThread] running the remaining "atexit" finalizers
```

pytest output after fix:
```
======================================================================================= test session starts ========================================================================================
platform linux -- Python 3.6.6, pytest-3.7.2, py-1.5.4, pluggy-0.7.1
rootdir: /home/james/code/external_tools/joblib, inifile: setup.cfg
collected 1070 items / 2 skipped                                                                                                                                                                   

joblib/__init__.py s                                                                                                                                                                         [  0%]
joblib/parallel.py ss                                                                                                                                                                        [  0%]
joblib/test/test_backports.py s....                                                                                                                                                          [  0%]
joblib/test/test_disk.py .........                                                                                                                                                           [  1%]
joblib/test/test_format_stack.py s...                                                                                                                                                        [  1%]
joblib/test/test_func_inspect.py s....................................                                                                                                                       [  5%]
joblib/test/test_hashing.py sssssssssss..................................................................................................................................................... [ 20%]
............................................................................................................................................................................................ [ 37%]
....................................................................................................................................................ss.............                          [ 53%]
joblib/test/test_logger.py .                                                                                                                                                                 [ 53%]
joblib/test/test_memmapping.py ssssssssssssss..                                                                                                                                              [ 54%]
joblib/test/test_memory.py ss.........................................                                                                                                                       [ 58%]
joblib/test/test_my_exceptions.py ...                                                                                                                                                        [ 59%]
joblib/test/test_numpy_pickle.py sssssssssssssssssssss................................................................................s.                                                     [ 68%]
joblib/test/test_numpy_pickle_compat.py .                                                                                                                                                    [ 68%]
joblib/test/test_numpy_pickle_utils.py ..                                                                                                                                                    [ 68%]
joblib/test/test_parallel.py sssss.......................................................................................................................................................... [ 83%]
..................................................................................................................................................................ss                         [ 99%]
joblib/test/test_store_backends.py ...                                                                                                                                                       [ 99%]
joblib/test/test_testing.py .....                                                                                                                                                            [ 99%]
joblib/test/data/create_numpy_pickle.py s                                                                                                                                                    [100%]

============================================================================= 1005 passed, 67 skipped in 52.61 seconds =============================================================================
[INFO:MainProcess:MainThread] process shutting down
[DEBUG:MainProcess:MainThread] running all "atexit" finalizers with priority >= 0
[DEBUG:MainProcess:MainThread] Interpreter shutting down. Waking up queue_manager_threads [(<Thread(QueueManagerThread, started daemon 139697284175616)>, <joblib.externals.loky.process_executor._ThreadWakeup object at 0x7f0df8e796d8>)]
[DEBUG:MainProcess:QueueManagerThread] queue management thread shutting down
[DEBUG:MainProcess:QueueManagerThread] closing call_queue
[DEBUG:MainProcess:QueueManagerThread] telling queue thread to quit
[DEBUG:MainProcess:QueueManagerThread] joining processes
[DEBUG:MainProcess:QueueFeederThread] feeder thread got sentinel -- exiting
[DEBUG:MainProcess:QueueManagerThread] queue management thread clean shutdown of worker processes: []
[DEBUG:MainProcess:MainThread] running the remaining "atexit" finalizers
```
collijk: I'd have written some more extensive tests, but `filter_args` is a pretty hairy and hard to test function.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index b048c67..77813d2 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -13,6 +13,12 @@ Loïc Estève
     Fix Memory, MemorizedFunc and MemorizedResult round-trip pickling +
     unpickling (#746).
 
+James Collins
+
+    Fixed a regression in Memory when positional arguments are called as
+    kwargs several times with different values (#751).
+
+
 Release 0.12.2
 --------------
 
diff --git a/joblib/func_inspect.py b/joblib/func_inspect.py
index e9320b4..d4d2670 100644
--- a/joblib/func_inspect.py
+++ b/joblib/func_inspect.py
@@ -275,7 +275,7 @@ def filter_args(func, ignore_lst, args=(), kwargs=dict()):
         else:
             position = arg_position - len(arg_names)
             if arg_name in kwargs:
-                arg_dict[arg_name] = kwargs.pop(arg_name)
+                arg_dict[arg_name] = kwargs[arg_name]
             else:
                 try:
                     arg_dict[arg_name] = arg_defaults[position]


Test patch:
diff --git a/joblib/test/test_func_inspect.py b/joblib/test/test_func_inspect.py
index baccecc..86e297c 100644
--- a/joblib/test/test_func_inspect.py
+++ b/joblib/test/test_func_inspect.py
@@ -223,6 +223,18 @@ def test_filter_args_error_msg(exception, regex, func, args):
     excinfo.match(regex)
 
 
+def test_filter_args_no_kwargs_mutation():
+    """None-regression test against 0.12.0 changes.
+
+    https://github.com/joblib/joblib/pull/75
+
+    Make sure filter args doesn't mutate the kwargs dict that gets passed in.
+    """
+    kwargs = {'x': 0}
+    filter_args(g, [], [], kwargs)
+    assert kwargs == {'x': 0}
+
+
 def test_clean_win_chars():
     string = r'C:\foo\bar\main.py'
     mangled_string = _clean_win_chars(string)
diff --git a/joblib/test/test_memory.py b/joblib/test/test_memory.py
index b333a48..b3ac081 100644
--- a/joblib/test/test_memory.py
+++ b/joblib/test/test_memory.py
@@ -348,6 +348,26 @@ def test_memory_ignore(tmpdir):
     assert len(accumulator) == 1
 
 
+def test_memory_args_as_kwargs(tmpdir):
+    """Non-regression test against 0.12.0 changes.
+
+    https://github.com/joblib/joblib/pull/751
+    """
+    memory = Memory(location=tmpdir.strpath, verbose=0)
+
+    @memory.cache
+    def plus_one(a):
+        return a + 1
+
+    # It's possible to call a positional arg as a kwarg.
+    assert plus_one(1) == 2
+    assert plus_one(a=1) == 2
+
+    # However, a positional argument that joblib hadn't seen
+    # before would cause a failure if it was passed as a kwarg.
+    assert plus_one(a=2) == 3
+
+
 @parametrize('ignore, verbose, mmap_mode', [(['x'], 100, 'r'),
                                             ([], 10, None)])
 def test_partial_decoration(tmpdir, ignore, verbose, mmap_mode):