Implemented logic to add extra arguments to apply by gabrielelanaro · Pull Request #3256 · dask/dask

gabrielelanaro · 2018-03-08T18:23:00Z

Note this change could break existing code because meta becomes just an
extra keyword with no positional meaning.

Tests added / passed
Passes flake8 dask
Fully documented, including docs/source/changelog.rst for all changes
and one of the docs/source/*-api.rst files for new API

Note this change could break existing code because meta becomes just an extra keyword

mrocklin

This looks pretty good to me. I left a couple of minor comments. Can I also ask you to add a note in the changelog at docs/source/changelog.rst ? If this is your first time then you will also have to add your name and a link at the bottom of that document.

mrocklin · 2018-03-08T18:25:47Z

dask/dataframe/tests/test_groupby.py

+        return df.assign(b=df.b - df.b.mean() + c * d)
+
+    assert_eq(df.groupby('a').apply(func, 1),
+              ddf.groupby('a').apply(func, 1))


Can we also test the keyword argument d= in another line?

mrocklin · 2018-03-08T18:26:15Z

dask/dataframe/tests/test_groupby.py

+    ddf = dd.from_pandas(df, npartitions=3)
+
+    pytest.raises(Exception, lambda: df.groupby('does_not_exist'))
+    pytest.raises(Exception, lambda: df.groupby('a').does_not_exist)


Should these raise particular kinds of exceptions, like AttributeError ?

martindurant · 2018-03-08T18:31:42Z

dask/dataframe/groupby.py


    @insert_meta_param_description(pad=12)
-    def apply(self, func, meta=no_default):
+    def apply(self, func, *args, **kwargs):


Is there any reasonable way to check that args[0] is in fact a meta and give deprecation warning? In py3 you can have positionally default argument, that would work too to avoid API breakage.

Precisely, this really bugs me.

But also with positially default arguments, I can totally see people do it in this way:

df.groupby().apply(f, arg1, arg2)

And fail miserably.

Another way is to make meta a required argument.

I'm fine with having things fail here, people really should be using keyword arguments by name instead of by position.

gabrielelanaro · 2018-03-08T18:32:42Z

@mrocklin thanks for reviewing.

There's also an extra issue that bugs me.

Let's say we pass a Future in args or kwargs, because you have a large argument I did have some issues especially with meta estimation. I will add a test-case to illustrate the issue and maybe we can find a way.

mrocklin · 2018-03-08T18:48:52Z

Let's say we pass a Future in args or kwargs, because you have a large argument I did have some issues especially with meta estimation. I will add a test-case to illustrate the issue and maybe we can find a way.

Yes, I agree both that that is important, and that it may be challenging. There are probably two challenges here:

Getting the task graph of a delayed or dd.Scalar object, and merging its task graph into the computation. This is straightforward, and has been done many other places in the codebase, but might take a bit of time if it's your first time (although you seem to be handling everything here very well :))
Getting metadata. In principle you would have to run the computation, which probably isn't feasible as a default. I think that in this case we probably need to force the user to provide meta=

gabrielelanaro · 2018-03-08T20:45:31Z

@mrocklin I will make the test pass, for now I'll skip the metadata issue.

mrocklin · 2018-03-08T21:01:39Z

dask/dataframe/tests/test_groupby.py

+    c = 1
+    d = 2
+    c_scalar = dd.core.Scalar({'my-scalar': c}, 'my-scalar', int)
+    d_scalar = dd.core.Scalar({'my-scalar': d}, 'my-scalar', int)


Rather than use internal API I recommend that we us something like df.a.sum(). This will make this test less brittle to internal changes and make it easier for novice maintainers to understand.

mrocklin · 2018-03-08T21:01:51Z

Nice test!

mrocklin · 2018-03-09T13:45:56Z

dask/dataframe/tests/test_groupby.py

+    d = 2
+
+    c_scalar = _make_scalar(c)
+    d_scalar = _make_scalar(d)


I suggest the following instead:

c_scalar = ddf.a.sum() d_scalar = ddf.b.mean()

gabrielelanaro · 2018-03-10T23:08:39Z

I'm having some issues dealing with delayed kwargs. I believe these don't get replaced in apply_and_enforced, I used a workaround, let me know if it looks OK.

mrocklin · 2018-03-10T23:44:02Z

Have you taken a look at dask.delayed.to_task_dask?

def to_task_dask(expr):
    """Normalize a python object and merge all sub-graphs.

    - Replace ``Delayed`` with their keys
    - Convert literals to things the schedulers can handle
    - Extract dask graphs from all enclosed values

    Parameters
    ----------
    expr : object
        The object to be normalized. This function knows how to handle
        ``Delayed``s, as well as most builtin python types.

    Returns
    -------
    task : normalized task to be run
    dask : a merged dask graph that forms the dag for this task

    Examples
    --------
    >>> a = delayed(1, 'a')
    >>> b = delayed(2, 'b')
    >>> task, dask = to_task_dask([a, b, 3])
    >>> task  # doctest: +SKIP
    ['a', 'b', 3]
    >>> dict(dask)  # doctest: +SKIP
    {'a': 1, 'b': 2}

    >>> task, dasks = to_task_dask({a: 1, b: 2})
    >>> task  # doctest: +SKIP
    (dict, [['a', 1], ['b', 2]])
    >>> dict(dask)  # doctest: +SKIP
    {'a': 1, 'b': 2}

gabrielelanaro · 2018-03-11T00:28:27Z

@mrocklin It's possible to replace the kwargs handling with a task using to_dask_task. There's however another test that conflicts with both strategies.

Is there any specific reason for the test or I can edit?

    def test_map_partitions_keeps_kwargs_in_dict():
        df = pd.DataFrame({'x': [1, 2, 3, 4], 'y': [5, 6, 7, 8]})
        a = dd.from_pandas(df, npartitions=2)
    
        def f(s, x=1):
            return s + x
    
        b = a.x.map_partitions(f, x=5)
    
>       assert "'x': 5" in str(b.dask)

mrocklin · 2018-03-11T00:35:46Z

It seems perfectly reasonable to change that test. My guess is that the intent behind that test is that we maintain keyword arguments in the task graph directly in some easy-to-interpret way, rather than including them as closures or something else more exotic.

mrocklin · 2018-03-11T15:48:07Z

This looks good to me, but @jcrist might have a better eye for issues here. Lets give him 24 hours to see if he has time to respond (he may be out of contact for the next few days).

gabrielelanaro · 2018-03-15T17:12:06Z

@mrocklin @jcrist any update on this PR?

jcrist

Apologies for the delayed review, overall this looks good to me.

jcrist · 2018-03-15T17:16:26Z

dask/dataframe/core.py

        func : function
            Function applied to each partition.
-        args, kwargs :
+        args, kwargs : Scalar, Delayed or object


args and kwargs can contain these types, but aren't these types themselves. I'd remove the type note here, and move it to the description below. Something like Arguments and keywords may contain ``Scalar``, ``Delayed``, regular python objects.

jcrist · 2018-03-15T17:17:53Z

dask/dataframe/core.py


    Ensures the output has the same columns, even if empty."""
-    df = func(*args, **kwargs)
+    df = func(*args, **dict(kwargs))


Why this change? I don't think this should be necessary.

It was something I forgot to clean up, I'll remove it

jcrist · 2018-03-15T17:19:42Z

dask/dataframe/core.py

    name = '{0}-{1}'.format(name, token)

    from .multi import _maybe_align_partitions
+    args, args_dasks = _process_lazy_args(args)


I think this can be moved down to where the call to to_task_dask is with no issues (since alignment also ignores Delayed objects). I'd prefer to group those together if possible.

jcrist · 2018-03-15T17:21:32Z

dask/dataframe/groupby.py


    @insert_meta_param_description(pad=12)
-    def apply(self, func, meta=no_default):
+    def apply(self, func, *args, **kwargs):


I'm fine with having things fail here, people really should be using keyword arguments by name instead of by position.

jcrist · 2018-03-18T21:09:06Z

LGTM. Thanks @gabrielelanaro! Merging.

Implemented logic to add extra arguments to apply

bd9cce2

Note this change could break existing code because meta becomes just an extra keyword

mrocklin reviewed Mar 8, 2018

View reviewed changes

martindurant reviewed Mar 8, 2018

View reviewed changes

Implemented dd.core.Scalar test

87908b9

mrocklin reviewed Mar 8, 2018

View reviewed changes

Updated tests with/without meta

4e34c84

mrocklin reviewed Mar 9, 2018

View reviewed changes

Partial implementation, trouble with keyword arguments

41e1eb7

gabrielelanaro added 4 commits March 10, 2018 15:19

Fixed delayed implementation

e0515f8

Docstrings

c4e19e9

Added changelog

b6d4579

Merge remote-tracking branch 'upstream/master' into issue-3234

12b085a

Make flake8 happy

cb76392

Reuse of to_task_dask and test adaptation

7b7be8e

gabrielelanaro changed the title ~~WIP: Implemented logic to add extra arguments to apply~~ Implemented logic to add extra arguments to apply Mar 11, 2018

jcrist reviewed Mar 15, 2018

View reviewed changes

Refining docstrings, general cleanup

c031600

jcrist merged commit 9e5267f into dask:master Mar 18, 2018

jcrist mentioned this pull request Mar 18, 2018

Make _Groupby.apply take additional arguments. #3234

Closed

jcrist mentioned this pull request Mar 18, 2018

dataframe map_partitions doesn't resolve future arguments #3236

Closed

Uh oh!

Conversation

gabrielelanaro commented Mar 8, 2018 • edited by mrocklin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielelanaro commented Mar 8, 2018

Uh oh!

mrocklin commented Mar 8, 2018

Uh oh!

gabrielelanaro commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrocklin commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielelanaro commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Mar 10, 2018

Uh oh!

gabrielelanaro commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Mar 11, 2018

Uh oh!

mrocklin commented Mar 11, 2018

Uh oh!

gabrielelanaro commented Mar 15, 2018

Uh oh!

jcrist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcrist commented Mar 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gabrielelanaro commented Mar 8, 2018 •

edited by mrocklin

Loading

gabrielelanaro commented Mar 8, 2018 •

edited

Loading

gabrielelanaro commented Mar 10, 2018 •

edited

Loading

gabrielelanaro commented Mar 11, 2018 •

edited

Loading