Generically rebuild a collection with different keys #7142

crusaderky · 2021-01-29T19:05:53Z

commit 2ceaf46 Author: crusaderky <[email protected]> Date: Thu Jan 28 18:53:42 2021 +0000 polish commit 2643032 Author: crusaderky <[email protected]> Date: Thu Jan 28 17:13:57 2021 +0000 black commit 1b29cd8 Author: crusaderky <[email protected]> Date: Thu Jan 28 17:13:45 2021 +0000 bugfix commit 0a50ceb Merge: 8286a36 9bb586a Author: crusaderky <[email protected]> Date: Thu Jan 28 16:48:34 2021 +0000 Merge remote-tracking branch 'upstream/master' into graphsurgery commit 8286a36 Author: crusaderky <[email protected]> Date: Thu Jan 28 16:48:12 2021 +0000 bugfix commit 802ece7 Author: crusaderky <[email protected]> Date: Thu Jan 28 16:22:47 2021 +0000 bugfixes commit 8d31b85 Author: crusaderky <[email protected]> Date: Thu Jan 28 15:38:33 2021 +0000 Drop unused layers commit 966d5a7 Author: crusaderky <[email protected]> Date: Thu Jan 28 15:20:42 2021 +0000 bugfixes commit afdc65d Merge: 1b30c21 5694253 Author: crusaderky <[email protected]> Date: Tue Jan 26 19:00:35 2021 +0000 Merge remote-tracking branch 'upstream/master' into graphsurgery commit 1b30c21 Author: crusaderky <[email protected]> Date: Tue Jan 26 14:07:59 2021 +0000 polish commit 4d03052 Author: crusaderky <[email protected]> Date: Tue Jan 26 13:46:02 2021 +0000 bind implementation commit 741b0e4 Author: crusaderky <[email protected]> Date: Tue Jan 26 11:30:12 2021 +0000 fix commit 5005c60 Author: crusaderky <[email protected]> Date: Tue Jan 26 11:26:46 2021 +0000 refactor commit 050aac8 Merge: c68a459 fbbc271 Author: crusaderky <[email protected]> Date: Tue Jan 26 11:16:05 2021 +0000 Merge remote-tracking branch 'upstream/master' into graphsurgery commit c68a459 Author: crusaderky <[email protected]> Date: Mon Jan 25 21:57:43 2021 +0000 bind implementation commit 6589c18 Merge: 7ac6817 4e2d838 Author: crusaderky <[email protected]> Date: Mon Jan 25 21:24:05 2021 +0000 Merge remote-tracking branch 'upstream/master' into graphsurgery commit 7ac6817 Author: crusaderky <[email protected]> Date: Mon Jan 25 21:23:53 2021 +0000 Rename choke -> checkpoint commit deec2b3 Author: crusaderky <[email protected]> Date: Mon Jan 25 21:19:49 2021 +0000 Rename graphsurgery -> graph_manipulation commit 60e928d Author: crusaderky <[email protected]> Date: Mon Jan 25 21:18:27 2021 +0000 more bind implementation commit fe17526 Author: crusaderky <[email protected]> Date: Mon Jan 25 13:55:54 2021 +0000 implement bind (partial) d commit 515f64f Author: crusaderky <[email protected]> Date: Mon Jan 25 11:33:17 2021 +0000 polish commit 4046708 Merge: 16fbb10 5c96d82 Author: crusaderky <[email protected]> Date: Mon Jan 25 10:43:51 2021 +0000 Merge remote-tracking branch 'dask/master' into graphsurgery commit 16fbb10 Author: crusaderky <[email protected]> Date: Mon Jan 25 10:35:21 2021 +0000 Implement name= commit d0f0348 Author: crusaderky <[email protected]> Date: Fri Jan 22 18:15:34 2021 +0000 postpersist name= argument commit c1d695f Author: crusaderky <[email protected]> Date: Fri Jan 22 18:15:17 2021 +0000 bind implementation (incomplete) commit 1ae20fe Author: crusaderky <[email protected]> Date: Fri Jan 22 14:44:20 2021 +0000 polish commit a30f0ed Author: crusaderky <[email protected]> Date: Fri Jan 22 14:34:25 2021 +0000 polish commit b979ea2 Merge: 793df1c 4395973 Author: crusaderky <[email protected]> Date: Fri Jan 22 14:05:49 2021 +0000 Merge remote-tracking branch 'dask/master' into graphsurgery commit 793df1c Author: crusaderky <[email protected]> Date: Fri Jan 22 14:05:42 2021 +0000 impl commit 1f66b9a Merge: ba43137 cb9fc26 Author: crusaderky <[email protected]> Date: Thu Jan 21 17:55:44 2021 +0000 Merge remote-tracking branch 'dask/master' into graphsurgery commit ba43137 Author: crusaderky <[email protected]> Date: Thu Jan 21 17:55:30 2021 +0000 graphsurgery (incomplete) commit 0ad5fe3 Author: crusaderky <[email protected]> Date: Thu Jan 21 14:04:59 2021 +0000 O(1) access to keys in HighLevelGraph commit 405205a Merge: b4a7197 30c9a3a Author: crusaderky <[email protected]> Date: Thu Jan 21 10:59:55 2021 +0000 Merge remote-tracking branch 'dask/master' into graphsurgery commit b4a7197 Author: crusaderky <[email protected]> Date: Wed Jan 20 21:00:48 2021 +0000 drop prototype bugfix

More tests More tests

jrbourbeau

Thanks for the PR and thoroughly testing / documenting the changes here @crusaderky!

cc @jcrist @eriknw @shoyer for visibility

jrbourbeau · 2021-02-01T16:19:36Z

dask/bag/core.py

+        return self._rebuild, ()
+
+    def _rebuild(self, dsk, name=None):
+        return type(self)(dsk, name or self.name, self.npartitions)


Out of curiosity, do we need to use type(self) here instead of Bag? I noticed we mostly hard-code the class type in other _rebuild methods

For historical reasons, dask bag supports subclassing, so the type needs to be generic.

jrbourbeau · 2021-02-01T16:30:58Z

dask/tests/test_base.py

+        if not isinstance(other, Tuple):
+            return NotImplemented  # pragma: nocover
+        name = tokenize(self, other)
+        dsk = {(name, i): k for i, k in enumerate(self._keys + other._keys)}
+        return Tuple(merge(dsk, self._dask, other._dask), list(dsk))


Thanks for aligning this with the updated key standard

jrbourbeau · 2021-02-01T16:35:12Z

dask/tests/test_base.py

-    assert t2._dask == dict(zip("abcdef", range(1, 7)))
-    assert t2.compute() == (1, 2, 3, 4, 5, 6)
-    x2, y2, z2 = dask.persist(x, y, z)
+    assert t2._keys == t._keys


It looks like we're no longer testing t2._dask here

That was because I had to complicate how Tuple.__add__ works to make it compliant to the new protocol spec. I now partially reintegrated the test.

jrbourbeau · 2021-02-01T16:44:11Z

dask/tests/test_base.py

+    ddf2 = ddf1.persist()
+    assert isinstance(ddf2, dd.DataFrame)
+    assert len(ddf2.__dask_graph__()) == 2
+    dd._compat.tm.assert_frame_equal(ddf2.compute(), ddf1.compute())


We usually use dd.utils.assert_eq (and the array and bag equivalent) for testing that collections are equal to one another. The assert_eq utilities call compute internally and perform a few additional sanity checks

jrbourbeau · 2021-02-01T16:48:22Z

docs/source/changelog.rst

 .. _`Stuart Berg`: https://github.com/stuarteberg
 .. _`Guillaume Eynard-Bontemps`: https://github.com/guillaumeeb
 .. _`Adam Beberg`: https://github.com/beberg
-.. _`Johnnie Gray`: https://github.com/jcmgray


Thanks for catching some changelog issues. The other updates look great, but I'm confused why this entry was removed.

Gotcha, thanks for clarifying 👍

docs/source/custom-collections.rst

docs/source/spec.rst

jrbourbeau · 2021-02-01T17:04:32Z

docs/source/custom-collections.rst

+    names : tuple
+        Tuple of names of the HighLevelGraph layers which contain all keys returned by
+        :meth:`__dask_keys__`.
+


Just checking, should we include the name= keyword in __dask_postpersist__ here or in #7109?

Here. I was convinced I added it already; fixed now.

Co-authored-by: James Bourbeau <[email protected]>

crusaderky · 2021-02-01T17:42:46Z

@jrbourbeau implemented requested changes

jrbourbeau

Looking through the custom collection docs, we'll need to update the example at https://docs.dask.org/en/latest/custom-collections.html#example-dask-collection too.

As this touches the task graph spec, let's give a bit more time for feedback from others. But overall the changes here seem reasonable.

dask/tests/test_base.py

crusaderky · 2021-02-02T12:13:17Z

@jrbourbeau updated https://docs.dask.org/en/latest/custom-collections.html#example-dask-collection

jrbourbeau

Thanks @crusaderky! This is in

eriknw · 2021-02-03T17:47:21Z

docs/source/spec.rst

+
+- a string, or
+- a tuple of one or more elements, whose first element is a string and the remainder are
+  arbitrary hashables:


Changing the definition of a key here seems unnecessary, and the old definition remains true. In practice, keys are typically strings or tuples as you describe, but this isn't enforced from what I see. For example, perhaps somebody paranoid would prefer to build a dask graph using objects of a Key class that they create. I haven't seen this done, but the previous spec definition allows for this. Dask collections (including custom collections) are not the only way to create dask graphs.

Updating the specification for keys of collections (the bulk of this PR) is a good idea.

* Add array_safe function supporting like= kwarg * Support for like= in percentile * Add CuPy tests for percentiles * Fix array_like_safe for NumPy < 1.20 * Improve percentile perf * Generically rebuild a collection with different keys (#7142) * Add array_safe function supporting like= kwarg * Support for like= in percentile * Add CuPy tests for percentiles * Fix array_like_safe for NumPy < 1.20 * Improve percentile perf * Remove note comparing np/merge_sorted * Update low-level graph spec to use any hashable for keys (#7163) * Remove unused merge_sorted Co-authored-by: Peter Andreas Entschev <[email protected]> Co-authored-by: Ashwin Srinath <[email protected]> Co-authored-by: crusaderky <[email protected]> Co-authored-by: James Bourbeau <[email protected]>

crusaderky added 6 commits January 28, 2021 18:54

Code review

89e1333

Forked from graph_manipulation PR

13587d1

test_base

52f799d

More tests More tests

Merge remote-tracking branch 'upstream/master' into postpersist

c356a3a

Docs

789d299

crusaderky mentioned this pull request Jan 29, 2021

graph manipulation: clone, bind, checkpoint, wait_on #7109

Merged

Merge branch 'master' into postpersist

075e65b

crusaderky closed this Feb 1, 2021

crusaderky reopened this Feb 1, 2021

jrbourbeau reviewed Feb 1, 2021

View reviewed changes

crusaderky and others added 5 commits February 1, 2021 17:26

Update docs/source/custom-collections.rst

1ce395c

Co-authored-by: James Bourbeau <[email protected]>

Update docs/source/custom-collections.rst

a6ca039

Co-authored-by: James Bourbeau <[email protected]>

Update docs/source/spec.rst

75866de

Co-authored-by: James Bourbeau <[email protected]>

Code review

3b6732e

Merge remote-tracking branch 'upstream/master' into postpersist

f11d3e8

jrbourbeau reviewed Feb 2, 2021

View reviewed changes

dask/tests/test_base.py Outdated Show resolved Hide resolved

dask/tests/test_base.py Outdated Show resolved Hide resolved

crusaderky added 2 commits February 2, 2021 12:12

Code review

b6c7960

Merge remote-tracking branch 'upstream/master' into postpersist

44b8b89

Fix documentation

fba0bd8

crusaderky force-pushed the postpersist branch from 56e62d6 to fba0bd8 Compare February 2, 2021 12:17

crusaderky added a commit to crusaderky/dask that referenced this pull request Feb 2, 2021

Squashed merge of dask#7142

ffd4eba

jrbourbeau approved these changes Feb 3, 2021

View reviewed changes

jrbourbeau merged commit d096fde into dask:master Feb 3, 2021

crusaderky deleted the postpersist branch February 3, 2021 17:07

eriknw reviewed Feb 3, 2021

View reviewed changes

shwina pushed a commit to shwina/dask that referenced this pull request Feb 5, 2021

Generically rebuild a collection with different keys (dask#7142)

b9c2968

keewis mentioned this pull request Feb 5, 2021

⚠️ Nightly upstream-dev CI failed ⚠️ pydata/xarray#4860

Closed

sjperkins mentioned this pull request Feb 8, 2021

Persisting datasets containing arrays with a single chunk fails pydata/xarray#4882

Closed

fjetter mentioned this pull request Jan 25, 2023

Race conditions in implicit creation of worker clients when serializing futures resulting in distributed.CancelledErrors dask/distributed#7498

Closed

Uh oh!

Generically rebuild a collection with different keys #7142

Generically rebuild a collection with different keys #7142

Uh oh!

Conversation

crusaderky commented Jan 29, 2021

Uh oh!

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

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Feb 1, 2021

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

crusaderky commented Feb 2, 2021

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants