Fix WorkItem.__hash__

[jrueb]

Currently getting the hash of a WorkItem will often result in a TypeError, even though we force the generation of a __hash__ method inside WorkItem using unsafe_hash=True. This is because WorkItem.usermeta is a dict when it's not None, and a dict is not hashable.

The solution is to exclude the usermeta from comparison, which in turn will also exclude it from the hashing.

[nsmith-]

Do you happen to know why our tests did not already cover this issue?

[jrueb]

There is also no test dedicated to test WorkItem and it seems currently WorkItems are normally kept inside lists in Coffea, so their hash method isn't used.

[lgray]

Is there any way we could turn the dicts into nested tuples? or do we think it is not relevant to hash all the metadata?

[lgray]

And, indeed, please add a test!

[jrueb]

Is there any way we could turn the dicts into nested tuples? or do we think it is not relevant to hash all the metadata?

Even if the usermeta is saved as tuples, probably the issue would arise again once somebody decides to put a value that is some non-hashable object inside usermeta.
Does a work item really become different just by having a different user meta data? If everything else is the same, it still defines the same task to be done, so to me excluding usermeta from hash and eq makes sense.

[lgray]

From the perspective of describing the data to be read in and processed I agree, that makes sense.

I was thinking to cases where some one is considering corrections or something else as metadata, rather than packing it with the processor. Perhaps we should have something that yells at people if they have some enormous piece of metadata for the dataset?

[jrueb]

With "enormous" do you mean something non-hashable?

[lgray]

Not necessarily, string blob data is perfectly well hashable.

[nsmith-]

Later on, the metadata is attached as a parameter to an awkward array object, so it must be json-serializable. One could check at the beginning if that is the case by trying to run json.dumps(metadata) and if it fails emit an exception. Nevertheless, I think at least in the current design the metadata should not be hashed to define the work item.

[lgray]

I guess then the question goes back to @jrueb... For what reason are you hashing the WorkItem? Maybe I missed it on the last PR.

I'm OK with omitting the user meta if it's not something that's normally done.

[nsmith-]

Initially I thought it was for the dask worker affinity, but looks like there I hash specific fields instead:

coffea/coffea/processor/executor.py

Lines 671 to 677 in 183cbe4

	def belongsto(heavy_input, workerindex, item):
	if heavy_input is not None:
	item = item[0]
	hashed = _hash(
	(item.fileuuid, item.treename, item.entrystart, item.entrystop)
	)
	return hashed % len(workers) == workerindex

[jrueb]

I guess then the question goes back to @jrueb... For what reason are you hashing the WorkItem? Maybe I missed it on the last PR.

Personally I would like to use the hash, or rather the ability to store the items in a set to search for them in a reasonable amount of time, in an Executor class I made.

[lgray]

OK if it's only those specific fields any way then that sorta settles it, we can skip the usermeta.
I suppose folks can complain later if it throws a wrench in their designs.