Ensure tokenize is consistent for pickle roundtrips #10808

fjetter · 2024-01-16T13:16:39Z

Closes Non deterministic tokenization for empty numpy arrays after pickle roundtrip #10799

It looks like the memory mapped arrays implemented different semantics than other arrays. I changed the implementation accordingly to also hash the data instead of using file metadata. Memory mapped arrays now tokenize the same as other arrays (e.g. when they point to the same data they will tokenize identically)

Beyond that, all tokenize calls are now testing pickle roundtrips.

This is particularly important for dask/dask-expr#14 where we technically need even stronger guarantees (same token for different processes) but I believe this should be sufficient.

Similar problems could already pop up with HLGs but they are likely a little less susceptible to this

github-actions · 2024-01-16T13:56:55Z

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

15 files ± 0 15 suites ±0 3h 20m 29s ⏱️ + 8m 0s
12 991 tests + 5 12 060 ✅ + 3 929 💤 ± 0 2 ❌ +2
160 552 runs +75 144 044 ✅ +93 16 505 💤 - 21 3 ❌ +3

For more details on these failures, see this check.

Results for commit a9b17c4. ± Comparison against base commit c2c1ece.

This pull request removes 12 and adds 17 tests. Note that renamed tests count towards both.

dask.tests.test_delayed ‑ test_name_consistent_across_instances
dask.tests.test_tokenize ‑ test_normalize_function_dataclass_field_no_repr
dask.tests.test_tokenize ‑ test_tokenize_datetime_date
dask.tests.test_tokenize ‑ test_tokenize_dense_sparse_array[bsr]
dask.tests.test_tokenize ‑ test_tokenize_dense_sparse_array[coo]
dask.tests.test_tokenize ‑ test_tokenize_dense_sparse_array[csc]
dask.tests.test_tokenize ‑ test_tokenize_dense_sparse_array[csr]
dask.tests.test_tokenize ‑ test_tokenize_dense_sparse_array[dia]
dask.tests.test_tokenize ‑ test_tokenize_dense_sparse_array[lil]
dask.tests.test_tokenize ‑ test_tokenize_function_cloudpickle
…

dask.tests.test_delayed ‑ test_deterministic_name
dask.tests.test_tokenize ‑ test_empty_numpy_array
dask.tests.test_tokenize ‑ test_local_objects
dask.tests.test_tokenize ‑ test_tokenize_callable_class
dask.tests.test_tokenize ‑ test_tokenize_circular_recursion
dask.tests.test_tokenize ‑ test_tokenize_dataclass_field_no_repr
dask.tests.test_tokenize ‑ test_tokenize_datetime_date[other0]
dask.tests.test_tokenize ‑ test_tokenize_datetime_date[other1]
dask.tests.test_tokenize ‑ test_tokenize_datetime_date[other2]
dask.tests.test_tokenize ‑ test_tokenize_local_functions
…

♻️ This comment has been updated with latest results.

crusaderky · 2024-01-16T14:21:02Z

dask/tests/test_base.py

+def test_empty_numpy_array():
+    arr = np.array([])
+    assert arr.strides
+    assert tokenize(arr) == tokenize(arr)


This looks like something that's useful to push into the tokenize wrapper

good call... will update the PR shortly

crusaderky

Test is now failing

fjetter · 2024-01-17T14:00:17Z

dask/utils.py

+            try:
+                impl = lk[cls2]
+            except KeyError:
+                pass
+            else:
+                if cls is not cls2:
+                    # Cache lookup
+                    lk[cls] = impl
+                return impl


The lazy registration is sometimes registering more specific types which the dispatch misses otherwise. I should probably add a dedicated test for this / break it into a dedicated PR.

This shows up when running some of the tests in isolation

fjetter · 2024-01-17T14:01:19Z

dask/tests/test_base.py

@@ -485,7 +556,7 @@ class BDataClass:
 def test_tokenize_dataclass():
    a1 = ADataClass(1)
    a2 = ADataClass(2)
-    assert tokenize(a1) == tokenize(a1)


This was actually broken and shadowed by the cache. Hence, the cache is cleared now. I corrected the dataclass implementation but unfortunately, I had to access some semi-private attributes. I think that it should be OK but this should be reviewed

fjetter · 2024-01-17T14:03:25Z

dask/base.py

+                # co.co_filename,
+                # co.co_firstlineno,
+                # co.co_flags,
+                # co.co_lnotab,


I modeled this after an issue that was opened a couple of months ago. I don't have a strong prefernence whether line numbers should count here. See test test_tokenize_local_functions

This somewhat overlaps with dask/dask-expr#689 cc @milesgranger

fjetter · 2024-01-25T13:38:54Z

So this got a little more intense than I thought.

Good news first: It looks like the "non determinism of cloudpickle" is actually not a thing or at least not relevant for the cases we're covering here. To ensure that we're getting the kind of token determinism we're hoping for, I added two additional kinds of roundtrips.

We roundtrip the same object just ordinarily and assert that the token is the same
we'll roundtrip the object through the distributed serializer and tokenize the deserialized object and compare to the original
Last but not least, we'll run this as well on another process, i.e. we'll spin up an actual cluster (we'll reuse one for the entire module, i.e. test runtime for this module is at 2s for me), generate the token over there and compare it with the local version

It looks like all the cases we have covered here are working for all three consistency levels.

Bad news: I had to patch up a lot. There will also be a small patch in distributed, i.e. one test here will fail until that is in. Some tokens will also be more complex than before, e.g. I ended up tokenizing even a type object (previously we often used str or __name__ so the tokens will be a little more nested which can cause the tokenization to degrade in performance. For now, I don't worry about that since I do not believe this to be substantial

fjetter · 2024-01-25T14:51:23Z

dask/distributed#8480 will be needed for CI to pass

dask/tests/test_tokenize.py

fjetter · 2024-01-26T15:00:08Z

Interestingly, in CI the local tokenization does fail but only sporadically 🤔

crusaderky · 2024-01-29T23:01:29Z

Status update

dask-expr CI [https://github.com/Remove lambda tokenization hack dask-expr#822] is very very red due to removal of dask-expr specific lambda tokenization. Need to figure out why the new one in dask/dask isn't fit for purpose and why none of the tests in test_tokenize.py are reproducing the issue. Also, why are the dask/dask tests with dask-expr enabled almost completely green?
Unknown objects are blindly passed to pickle4. This can be very slow for third-party wrappers around numpy or pandas or any other large data. We should do a follow-up where we move to pickle5 with buffers.
If pickle4 of unknown objects fails, we go through a very sophisticated slots/dict recursive introspection. I think this should be replaced with a blind cloudpickle like it happens with functions. I attempted to do slots/dict introspection BEFORE blindly calling pickle4 and it resulted in an extreme slowdown.
We're now pickling potentially a lot more unknown objects than before. A performance A/B test is in order.

Failing tests

Windows /3.10: FAILED dask/tests/test_tokenize.py::test_tokenize_object_with_recursion_error
mindeps: dask/tests/test_tokenize.py::test_local_objects
dask_expr: FAILED dask/dataframe/io/tests/test_parquet.py::test_parquet_pyarrow_write_empty_metadata_append

False positives

dask_expr: FAILED dask/dataframe/io/tests/test_parquet.py::test_pyarrow_filter_divisions (this one fails in main too)
Closes GPU Tokenization #6718

phofl · 2024-01-29T23:12:21Z

dask_expr: FAILED dask/dataframe/io/tests/test_parquet.py::test_pyarrow_filter_divisions (this one fails in main too)

This one is green now if you merge main

This reverts commit 1b1136d.

This reverts commit dbb9278.

This reverts commit 4098e7f.

crusaderky · 2024-02-13T16:55:49Z

Superseded by #10883

fjetter mentioned this pull request Jan 16, 2024

[WIP] Pass non HLG objects wout materialization #10369

Draft

crusaderky approved these changes Jan 16, 2024

View reviewed changes

crusaderky requested changes Jan 17, 2024

View reviewed changes

fjetter commented Jan 17, 2024

View reviewed changes

This was referenced Jan 18, 2024

add test that shows how lambda tokenization is broken dask/dask-expr#765

Closed

Use inspect to deterministically tokenize local/lambda functions dask/dask-expr#767

Closed

fjetter marked this pull request as draft January 19, 2024 08:32

fjetter force-pushed the ensure_tokenize_pickle_consistent branch from 1ca49b3 to 9cac38c Compare January 25, 2024 13:32

fjetter mentioned this pull request Jan 25, 2024

Ensure numpy matrix subclass roundtrips properly dask/distributed#8480

Merged

fjetter commented Jan 25, 2024

View reviewed changes

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

fjetter force-pushed the ensure_tokenize_pickle_consistent branch from 6b4e4db to c83b26f Compare January 26, 2024 10:43

fjetter mentioned this pull request Jan 26, 2024

Refactor: move tests for tokenize() to its own module #10863

Merged

crusaderky added a commit to crusaderky/dask that referenced this pull request Jan 26, 2024

Import rewrite of failing test from dask#10808

636cc1f

crusaderky added a commit to crusaderky/dask that referenced this pull request Jan 26, 2024

Import rewrite of failing test from dask#10808

69eb0a0

This was referenced Jan 29, 2024

Deterministic tokenize() for global random functions #10865

Merged

Remove lambda tokenization hack dask/dask-expr#822

Merged

crusaderky force-pushed the ensure_tokenize_pickle_consistent branch from 1d83bf0 to d0c846e Compare January 29, 2024 22:00

crusaderky mentioned this pull request Jan 29, 2024

Warn if tasks are submitted with identical keys but different run_spec dask/distributed#8185

Merged

crusaderky mentioned this pull request Jan 30, 2024

Make tokenization more deterministic #10876

Merged

crusaderky added a commit to fjetter/dask that referenced this pull request Jan 31, 2024

Reintroduce dask#10808

1b1136d

crusaderky added a commit to crusaderky/dask that referenced this pull request Jan 31, 2024

Revert "Reintroduce dask#10808"

dbb9278

This reverts commit 1b1136d.

crusaderky added a commit to crusaderky/dask that referenced this pull request Jan 31, 2024

Revert "Revert "Reintroduce dask#10808""

4098e7f

This reverts commit dbb9278.

crusaderky added a commit to crusaderky/dask that referenced this pull request Jan 31, 2024

Revert "Revert "Revert "Reintroduce dask#10808"""

690460e

This reverts commit 4098e7f.

crusaderky added a commit to crusaderky/dask that referenced this pull request Jan 31, 2024

Revert "Revert "Revert "Reintroduce dask#10808"""

7184889

This reverts commit 4098e7f.

crusaderky force-pushed the ensure_tokenize_pickle_consistent branch from ffbc27d to 1d4b876 Compare February 1, 2024 14:55

This was referenced Feb 1, 2024

Deterministic hashing for almost everything #10883

Merged

Remove redundant normalize_token variants #10884

Merged

crusaderky closed this Feb 1, 2024

crusaderky reopened this Feb 1, 2024

crusaderky force-pushed the ensure_tokenize_pickle_consistent branch 4 times, most recently from 8e8f560 to 06526e5 Compare February 5, 2024 15:31

crusaderky added 2 commits February 6, 2024 09:31

Make tokenization more deterministic

0f07cdf

Delta from dask#10876

a9b17c4

crusaderky force-pushed the ensure_tokenize_pickle_consistent branch from 06526e5 to a9b17c4 Compare February 6, 2024 09:32

This was referenced Feb 6, 2024

Test numba tokenization #10896

Merged

Tokenization meta-issue #10905

Closed

crusaderky closed this Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure tokenize is consistent for pickle roundtrips #10808

Ensure tokenize is consistent for pickle roundtrips #10808

fjetter commented Jan 16, 2024

github-actions bot commented Jan 16, 2024 •

edited

crusaderky Jan 16, 2024

fjetter Jan 17, 2024

crusaderky left a comment

fjetter Jan 17, 2024

fjetter Jan 17, 2024

fjetter Jan 17, 2024

fjetter commented Jan 25, 2024

fjetter commented Jan 25, 2024

fjetter commented Jan 26, 2024

crusaderky commented Jan 29, 2024 •

edited

phofl commented Jan 29, 2024

crusaderky commented Feb 13, 2024

Ensure tokenize is consistent for pickle roundtrips #10808

Ensure tokenize is consistent for pickle roundtrips #10808

Conversation

fjetter commented Jan 16, 2024

github-actions bot commented Jan 16, 2024 • edited

Unit Test Results

crusaderky Jan 16, 2024

Choose a reason for hiding this comment

fjetter Jan 17, 2024

Choose a reason for hiding this comment

crusaderky left a comment

Choose a reason for hiding this comment

fjetter Jan 17, 2024

Choose a reason for hiding this comment

fjetter Jan 17, 2024

Choose a reason for hiding this comment

fjetter Jan 17, 2024

Choose a reason for hiding this comment

fjetter commented Jan 25, 2024

fjetter commented Jan 25, 2024

fjetter commented Jan 26, 2024

crusaderky commented Jan 29, 2024 • edited

Status update

Failing tests

phofl commented Jan 29, 2024

crusaderky commented Feb 13, 2024

github-actions bot commented Jan 16, 2024 •

edited

crusaderky commented Jan 29, 2024 •

edited