Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add function source code to hash #35

Closed
wants to merge 1 commit into from

Conversation

jmrichardson
Copy link
Contributor

Enables function source code to be hashed. I haven't done extensive testing but seems to be working for the function that is decorated. #34 #13

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #35 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   95.67%   95.70%   +0.02%     
==========================================
  Files           4        4              
  Lines         324      326       +2     
  Branches       30       30              
==========================================
+ Hits          310      312       +2     
  Misses          8        8              
  Partials        6        6              
Impacted Files Coverage Δ
cachier/core.py 94.64% <100.00%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d56ca0c...46b00ed. Read the comment docs.

@shaypal5
Copy link
Collaborator

Hey @jmrichardson ,

Thank you for the submission, but this kinds of huge change really needs to be rigorously tested before I can consider adding it, while at the moment you have added no tests to check this.

The most important check, is to make sure that you never get "false positives"; i.e. changes to function hash without a change to the function. This has to be checked cross-kernel.

The second is how to handle soft false positives. For example, what about changes to function documentation, in-function in-line comments etc.? People would hardly expect a change that does not affect functionality to practically reset their function cache.

The third is opt-in configurability. This CAN'T be the default. Consider that adding prints or changing variables names or changing spacing or indentation will change the code. For some people who might otherwise rely on this to not trigger huge calculations, even after code changes, triggering a cache reset might be unexpected. I think documenting how this can be turned on with a configuration value is adequate.

Finally, false negatives also need to be tested; meaning, making sure we don't encounter cases where code changes somehow do not trigger hash changes.

Also, this needs to be documented. :)

What say you?

@shaypal5 shaypal5 self-requested a review September 15, 2020 14:48
@shaypal5 shaypal5 self-assigned this Sep 15, 2020
@shaypal5
Copy link
Collaborator

Also, see issues #18 and #19 , which brought about the rollback of PR #17 , since the used hash function turned out to have way to many collisions.

You will have to somehow check for this statistically and rigorously before I can be convinced to pull this in, as I'm not planning to add code just to have to roll it back.

Please make sure to address this.

I'm closing this PR. Please reopen, or open a new one, after you address all of the above issues.

Feel free to continue the discussion either here or in the issue you open for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants