Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Plugin: cache hook with
diskcache
#684Plugin: cache hook with
diskcache
#684Changes from 15 commits
37c4fb4
9d15a7a
51be118
961aa5f
ed46976
5dfe25d
d92e78e
f68ba55
162babe
dc330c3
d9e5205
47cf088
e195e1f
4040708
e48f085
f1fac0a
c0c3747
1604247
315c4e7
894de21
68ff666
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this function, the
CacheHook
doesn't work in notebooks because it won't be able to useinspect
to get the source code and create a hash.The alternative would be to use the functions's
__code__
attribute, but this would dependent on the Python version. It might not be an issue because the cache's pickling is already Python version dependentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used? Think I'm missing it... But that makes sense I think. You're talking about temporary modules, right?
With temporary modules you'd have to special-case them, go through every function and use
inspect
on them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might not be worth removing comments TBH -- can you at least add some comments here as to what you're doing? Code is a bit cryptic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth adding a test for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavior is tested via
test_graph_utils::
test_different_hash_docstring
andtest_different_hash_comment
. They check both if "the hashes are unequal when not stripping docs and comments" and "the hashes are equal when stripping docs and comments".The tests would be improved by not relying on
graph_utils.hash_source_code()
thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add comments to the function
I think the value of removing docstring/comments will depend on the workflow for the CI / CLI code diff (which rely on the same hashing).
If we marked as "changed" a node that had a changed docstring or added #comment AND all downstream nodes, the amount of false positive will make the feature pointless IMHO