`key_hash` function needs to be renamed, as the `key` is not hashed, just the `value`

[ttngu207]

datajoint-python/datajoint/hash.py

Line 7 in 6c92598

def key_hash(key):

Suggestion:

def key_hash(key):
    """
    32-byte hash used for lookup of primary keys of jobs
    """
    hashed = hashlib.md5()
    for k, v in sorted(key.items()):
        hashed.update(str(k).encode())
        hashed.update(str(v).encode())
    return hashed.hexdigest()

[dimitri-yatsenko]

Why?

[eywalker]

The key hash is really not uniquely hashing based on the whole dictionary but rather just based on values. While the chance of this colliding within the purpose of jobs table is low, just as a function, it's not staying true to its name in hashing the key. Key hash ought to include the dict key and value both in hash generation.

[dimitri-yatsenko]

The primary key of the jobs table is the table name + the key hash. The key names are not necessary to uniquely identify key values.

[eywalker]

Again, it's not needed for jobs, but it just makes more sense that this hash actually yields unique hash for actually distinct dict key. I don't see any reason why not and it makes this function potential useful outside of the limited use within jobs table.

[dimitri-yatsenko]

This particular function is meant to distinguish dicts with the same set of key names as it was designed. I would rather not change the computation of the jobs keys because this will job reservation issues in a group that only partially upgrades to the new version.

[eywalker]

Ok I buy that argument. Let's move this into jobs so that same implementation is used but we can provide for more generic hashing function incorporating keys within hash.

[dimitri-yatsenko]

Perhaps part of the confusion is that the word key here is meant to represent a table key value, not dict keys.

Yes, I agree that we can move this function to jobs.py or give it a better name to avoid confusion.