pass dask compute/persist args through from load/compute/perist by jhamman · Pull Request #1543 · pydata/xarray

jhamman · 2017-08-31T20:24:50Z

Closes Pass arguments to dask.compute() #1523
Tests added / passed
Passes git diff upstream/master | flake8 --diff
Fully documented, including whats-new.rst for all changes and api.rst for new API

cc @crusaderky - putting this up in case its useful to you.

I'm frankly not sure if my tests are doing what they need to but everything seems to be playing nicely.

crusaderky · 2017-09-01T07:41:13Z

xarray/core/variable.py

-        if not isinstance(self._data, np.ndarray):
+        if isinstance(self._data, dask_array_type):
+            self._data = np.asarray(self._data.compute(**kwargs))
+        elif not isinstance(self._data, np.ndarray):


This clause should be removed as it causes inconsistent behaviour with numpy scalar types. I cannot think of any other use case where data is neither a dask ARRAY nor a numpy ndarray?

I think this allows for on-disk type arrays. @shoyer, any thoughts on calling np.asarray here and in the line above?

Yes, we need this to support on-disk arrays that aren't backed by dask. (I'd love to get rid of this in favor of always using dask, but dask has some limitations that make this tricky.)

crusaderky · 2017-09-01T07:42:53Z

xarray/core/variable.py

        """
-        if not isinstance(self._data, np.ndarray):
+        if isinstance(self._data, dask_array_type):
+            self._data = np.asarray(self._data.compute(**kwargs))


You don't want to invoke asarray if dask returns a scalar numpy type

To be honest, we don't define what can go in _data as carefully as we ought to. I guess there are two ways to define it:

anything "array like" that defines at least shape, dtype and __getitem__

what comes out of xarray.core.variable.as_compatible_data

Numpy scalars do actually pass through here (since they define all those attributes!)... but then would get converted into an array when calling .values anyways:

xarray/xarray/core/variable.py

Lines 334 to 337 in 78ca20a

@property

def values(self):

"""The variable's data as a numpy.ndarray"""

return _as_array_or_item(self._data)

So I guess I agree, but on the other hand I'm also a little nervous that a dask routine might return a non-numpy scalar, which would definitely break if we don't wrap it in asarray. The safe thing to do is to leave this as is or call as_compatible_data on it.

crusaderky · 2017-09-01T07:45:55Z

xarray/tests/test_dask.py

+        b4 = b.persist(get=dask.multiprocessing.get, num_workers=4)
+        assert b4._in_memory
+        assert_equal(b4, expected)
+


Redundant with the test below?

fixed, these should have been Variables.

crusaderky · 2017-09-01T07:46:27Z

xarray/tests/test_dask.py

+
 def build_dask_array():
    global kernel_call_count
    kernel_call_count = 0


Missing tests for Variable

crusaderky · 2017-09-01T07:49:23Z

@jhamman happy to go with your version

shoyer · 2017-09-02T05:52:31Z

xarray/core/variable.py

-        if not isinstance(self._data, np.ndarray):
+        if isinstance(self._data, dask_array_type):
+            self._data = np.asarray(self._data.compute(**kwargs))
+        elif not isinstance(self._data, np.ndarray):


Yes, we need this to support on-disk arrays that aren't backed by dask. (I'd love to get rid of this in favor of always using dask, but dask has some limitations that make this tricky.)

shoyer · 2017-09-02T06:03:47Z

xarray/core/variable.py

        """
-        if not isinstance(self._data, np.ndarray):
+        if isinstance(self._data, dask_array_type):
+            self._data = np.asarray(self._data.compute(**kwargs))


To be honest, we don't define what can go in _data as carefully as we ought to. I guess there are two ways to define it:

anything "array like" that defines at least shape, dtype and __getitem__

what comes out of xarray.core.variable.as_compatible_data

Numpy scalars do actually pass through here (since they define all those attributes!)... but then would get converted into an array when calling .values anyways:

xarray/xarray/core/variable.py

Lines 334 to 337 in 78ca20a

@property

def values(self):

"""The variable's data as a numpy.ndarray"""

return _as_array_or_item(self._data)

So I guess I agree, but on the other hand I'm also a little nervous that a dask routine might return a non-numpy scalar, which would definitely break if we don't wrap it in asarray. The safe thing to do is to leave this as is or call as_compatible_data on it.

shoyer · 2017-09-02T06:37:40Z

xarray/tests/test_dask.py

+        expected = Variable('x', [1, 4])
+        b = a * a
+        # compute
+        b1 = b.compute(get=dask.multiprocessing.get)


The clean way to test this is probably with mock, e.g.,

In [63]: import numpy as np In [64]: import dask.array as da In [65]: from unittest import mock In [66]: x = da.from_array(np.arange(3), chunks=(2,)) In [67]: with mock.patch.object(da.Array, 'compute', return_value=np.arange(3)) as mock_compute: ...: x.compute(foo='bar') ...: In [68]: mock_compute.assert_called_with(foo='bar') In [69]: mock_compute.assert_called_with(bar='foo') --------------------------------------------------------------------------- AssertionError Traceback (most recent call last) <ipython-input-69-22937cf26ca1> in <module>() ----> 1 mock_compute.assert_called_with(bar='foo') ~/conda/envs/xarray-dev/lib/python3.5/unittest/mock.py in assert_called_with(_mock_self, *args, **kwargs) 792 if expected != actual: 793 cause = expected if isinstance(expected, Exception) else None --> 794 raise AssertionError(_error_message()) from cause 795 796 AssertionError: Expected call: compute(bar='foo') Actual call: compute(foo='bar')

unittest.mock is part of Python 3's standard library, but there's also a widely used Python 2 backport on pypi. I think it would be perfectly fine to add it as a dependency for our test suite.

Thanks @shoyer, I'll add mock as part of this PR.

…ompute_args

shoyer

I think the docs also need an update to mention that mock is a test-time requirement on Python 2.

shoyer · 2017-09-05T17:50:04Z

xarray/tests/test_dask.py

+    with mock.patch.object(Variable, method,
+                           return_value=np.arange(3)) as mock_method:
+        getattr(x, method)(foo='bar')
+    mock_method.assert_called_with(foo='bar')


I think you can skip this first test. This verifies that Variable.compute() was called if you directly call Variable.compute().

shoyer · 2017-09-05T17:51:08Z

xarray/tests/test_dask.py

 import numpy as np
 import pandas as pd
+import pytest
+import mock


On Python 3, use from unittest import mock to avoid adding the new dependency.

shoyer · 2017-09-05T18:10:39Z

Can you also update tests_require in setup.py?

xarray/setup.py

Line 39 in 9a8e2c5

TESTS_REQUIRE = ['pytest >= 2.7.1']

shoyer · 2017-09-05T18:15:49Z

xarray/tests/test_dask.py

 with suppress(ImportError):
    import dask
    import dask.array as da
+    import dask.multiprocessing


No longer used

pass dask compute/persist args through from load/compute/perist

7f0a3b8

crusaderky reviewed Sep 1, 2017

View reviewed changes

fix test and whatsnew note

df4f5d9

shoyer reviewed Sep 2, 2017

View reviewed changes

Joe Hamman added 5 commits September 4, 2017 21:19

Merge branch 'master' of github.com:pydata/xarray into feature/dask_c…

a186e70

…ompute_args

test dask compute args with mock

490784a

use as_compatible_data instead of np.asarray

a7af62a

requires dask

2b506c5

requires dask

a2dbe26

jhamman mentioned this pull request Sep 5, 2017

v0.10 Release #1535

Closed

13 tasks

shoyer reviewed Sep 5, 2017

View reviewed changes

cleanup tests and update docs

6fd941f

update setup.py

b5cc3bb

shoyer approved these changes Sep 5, 2017

View reviewed changes

cleanup imports

a879214

jhamman merged commit 5472fb5 into pydata:master Sep 5, 2017

jhamman deleted the feature/dask_compute_args branch September 5, 2017 19:55

	@property
	def values(self):
	"""The variable's data as a numpy.ndarray"""
	return _as_array_or_item(self._data)

Uh oh!

Conversation

jhamman commented Aug 31, 2017 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Sep 1, 2017

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!

shoyer 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!

shoyer commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jhamman commented Aug 31, 2017 •

edited

Loading