4 ms·

def hash_code(code): return hashlib.md5(code.encode()).hexdigest() Be warned. The above function is used as part of the hash. The ostensible purpose is t

by epr 3y ago

def hash_code(code): return hashlib.md5(code.encode()).hexdigest() Be warned. The above function is used as part of the hash. The ostensible purpose is to prevent using cached values of functions who's code has changed, but it does not handle dependencies of that function.

martinky24 3y ago
How do you suggest one might fix that issue? Also pin the cache to a hash of all dependency versions? And then if one minor update And let's say the dependency did change, but it's generally inert (more error handling around edge cases, for example), how do you factor that in? Blow up the whole cache? Your example isn't really a problem with OPs utility, but a specific example of a broader dependency management problem that affects just about everything. The answers usually boil down to 1) invest heavily in a kick ass test suite, 2) never upgrade or 3) upgrade and pray nothing breaks.
- williamzeng0 3y ago
  +1, we considered traversing the function's dependencies to key the cache on (not just the initial function source code), but decided to leave this in a as a constraint. Otherwise we also blowing up the cache when we didn't want it to happen.
- epr 3y ago
  > How do you suggest one might fix that issue? Also pin the cache to a hash of all dependency versions? Pretty much. Recursively collect dependencies by analyzing the AST of the code. > And then if one minor update And let's say the dependency did change, but it's generally inert (more error handling around edge cases, for example), how do you factor that in? Blow up the whole cache? You're saying that like it's some kind of ridiculous ask, but yes. The current implementation is already "Blow[ing] up the whole cache" whenever the code for the decorated function is changed anyways. I'd guess that additionally handling dependencies recursively would only modestly increase the rate of "Blow[ing] up the whole cache". > Your example isn't really a problem with OPs utility... Whether or not this is a problem in practice obviously depends on your use case. Maybe you don't generally care if functions return the correct result, but many do. > [This is] a specific example of a broader dependency management problem that affects just about everything. Dependency resolution is not trivial per se, but it's a pretty common problem. Every single package manager, build system (make), etc. have all solved this.
chlorion 3y ago
Using md5 for this seems like an odd choice. Sha1 is a better choice even for non-cryptographic use cases, it's quite a bit faster than md5. Even better would be something like xxhash! According to a quick bash script I wrote to benchmark the popular hash functions, md5 comes out last compared to sha1, sha256, sha512, and blake2, and by a decent margin! A good rule of thumb is to never use md5 at all. Not even for non-cryptographic use cases. It's not only broken, but also very slow!
- williamzeng0 3y ago
  That sounds great, I'm going to see how Sweep does on this issue: https://github.com/sweepai/sweep/issues/3333 https://github.com/sweepai/sweep/issues/3333
  - chlorion 3y ago
    I think python objects have a __hash__ method available on them as well that can be used for hashing. That should be even much faster than sha1, but for this use case I'm not sure how much it really matters. Would be interesting to benchmark!

by epr 3y ago

martinky24 3y ago

How do you suggest one might fix that issue? Also pin the cache to a hash of all dependency versions? And then if one minor update And let's say the dependency did change, but it's generally inert (more error handling around edge cases, for example), how do you factor that in? Blow up the whole cache? Your example isn't really a problem with OPs utility, but a specific example of a broader dependency management problem that affects just about everything. The answers usually boil down to 1) invest heavily in a kick ass test suite, 2) never upgrade or 3) upgrade and pray nothing breaks.

williamzeng0 3y ago
+1, we considered traversing the function's dependencies to key the cache on (not just the initial function source code), but decided to leave this in a as a constraint. Otherwise we also blowing up the cache when we didn't want it to happen.
epr 3y ago
> How do you suggest one might fix that issue? Also pin the cache to a hash of all dependency versions? Pretty much. Recursively collect dependencies by analyzing the AST of the code. > And then if one minor update And let's say the dependency did change, but it's generally inert (more error handling around edge cases, for example), how do you factor that in? Blow up the whole cache? You're saying that like it's some kind of ridiculous ask, but yes. The current implementation is already "Blow[ing] up the whole cache" whenever the code for the decorated function is changed anyways. I'd guess that additionally handling dependencies recursively would only modestly increase the rate of "Blow[ing] up the whole cache". > Your example isn't really a problem with OPs utility... Whether or not this is a problem in practice obviously depends on your use case. Maybe you don't generally care if functions return the correct result, but many do. > [This is] a specific example of a broader dependency management problem that affects just about everything. Dependency resolution is not trivial per se, but it's a pretty common problem. Every single package manager, build system (make), etc. have all solved this.

chlorion 3y ago

Using md5 for this seems like an odd choice. Sha1 is a better choice even for non-cryptographic use cases, it's quite a bit faster than md5. Even better would be something like xxhash! According to a quick bash script I wrote to benchmark the popular hash functions, md5 comes out last compared to sha1, sha256, sha512, and blake2, and by a decent margin! A good rule of thumb is to never use md5 at all. Not even for non-cryptographic use cases. It's not only broken, but also very slow!

williamzeng0 3y ago
That sounds great, I'm going to see how Sweep does on this issue: https://github.com/sweepai/sweep/issues/3333 https://github.com/sweepai/sweep/issues/3333
- chlorion 3y ago
  I think python objects have a __hash__ method available on them as well that can be used for hashing. That should be even much faster than sha1, but for this use case I'm not sure how much it really matters. Would be interesting to benchmark!