subs() comparing key hashes

[madsbk]

This PR makes subs() compare hashes of keys instead of comparing type and equality of tuple items.

This follows the graph specification: "A key is any hashable value that is not a task", which implies that different typed keys that hashes to the same value are the same key?.

This is motivated by my work on culling where keys containing int and numpy.int are substituted.

assert hash(('mykey', 42)) == hash(('mykey', np.int64(42)))

Finally, I expect this to be slightly faster than the current implementation.

• Tests added / passed
• Passes black dask / flake8 dask

[mrocklin]

Thanks @madsbk

cc'ing @eriknw who might have thoughts on this

[eriknw]

I wouldn't rely only on hashes, which seems far too risky. But you're also right that relying on types may be overly restrictive.

How about len({x, y}) == 1? This checks both hash and equality (as one would expect), and should be pretty fast.

[eriknw]

Aha, this is even better:

x in {y}

[eriknw]

This follows the graph specification: "A key is any hashable value that is not a task", which implies that different typed keys that hashes to the same value are the same key?.

To clarify, "hashable" does not imply only using the hash value. Typically, it also implies using equality as well, hence why I don't think we should only use hash values.

In general, I think it's probably okay to relax the type check. This definitely needs to go in the release notes, because I wouldn't be surprised if this change breaks some code somewhere (unlikely, but possible).

[madsbk]

To clarify, "hashable" does not imply only using the hash value. Typically, it also implies using equality as well, hence why I don't think we should only use hash values.

Make sense and I really like x in {y} !

[eriknw]

LGTM. I like it! Thanks @madsbk

[madsbk]

@mrocklin, I think this is ready to be merged.
I don't know what is going on with CI. It complains about black reformat of files I haven't touched. On my machine, both black and flake8 passes.

[mrocklin]

Black recently updated. I think that Tom resolved this on the Dask side with #6568 . Pushing an empty commit to trigger CI