How about implementing this at the level of ATen, with a specialization of std::hash for at::Device? Then the python-specific function could just return std::hash<at::Device>{}(device). It is a tiny bit more effort, but I could imagine we'd want to hash at::Device in C++ at one point or another, and then we'd have to move this code into ATen anyway. If you're comfortable with building/modifying ATen I would suggest this route, otherwise we can change this in another PR at a later point in time.

@pytorchbot retest this please

@pytorchbot retest this please

Actually a final improvement we could do would be to do a range check on the std::hash output before we static_cast it. This will prevent overflow errors, which can cause UB and make the hash function not conform to its invariants (e.g. hash of a single object can be different every time). You can use std::numeric_limits to find out what's the max value for Py_ssize_t (we're good with min because it's a signed type, so it's surely not larger than 0).

Sorry, I didn't think of that before 😕

I think this should be protected by the way the hash is designed. It is merely adding 3 to the device index, which is upper bounded by the limit of int32_t. I don't think there can be a situation where the hash could overflow, but I could add it, if you deem it necessary.

@vishwakftw yes, the current code is correct for sure, but what if for some reason we'll end up using hash_combine of multiple fields, and std::hash<int64_t>(index) in the future (e.g. because there will be more backends)? Who will remember to go and update the Python bindings because they have silent assumptions about the code that lives so far. The bug won't even be very apparent and will be very hard to debug.

Thank you for the detailed explanation. Should something like this do:

if (hash_val > std::numeric_limits<Py_ssize_t>::max()) {  // hash_val is size_t
  throw std::runtime_error("Hash value limit exceeded, can overflow");
}

No, that's kind of hard to fix for the user. Let's just use modulo arithmetic to limit the range.

Are you suggesting setting an upper bound for the device index, like in the issue ?

  return static_cast<Py_ssize_t>(std::hash<at::Device>{}(self->device) % MAX_HASH);

If this is how you want it done, where should I add MAX_HASH?

Sorry about too many questions.

Yeah, just use the numeric_limits to find out what's the maximal value. No worries, I'm happy to help.