feat: avoid polluting memory namespace when caching functions in jupyter notebooks#1570
feat: avoid polluting memory namespace when caching functions in jupyter notebooks#1570banteg wants to merge 1 commit into
Conversation
…ter notebooks while joblib previously correctly identified that part of the dynamically generated folder name for functions defined in a notebook depended on the pid and should be cut, it has missed that the filename was dynamically generated too and depended on the murmur2 hash of the function code. this has lead to pollution of memory namespace on each function change. instead, we collapse the ipykernel namespace to a single static part and let joblib.Memory handle the invalidation on code changes as it's supposed to do.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1570 +/- ##
==========================================
- Coverage 95.19% 95.13% -0.06%
==========================================
Files 45 45
Lines 7674 7674
==========================================
- Hits 7305 7301 -4
- Misses 369 373 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@lazer-dinosaur and @thisiswhereitype, since you commented in the associated issue, if you could test this PR inside a Jupyter notebook interface this would be great! It would be good to have a test for this 1 but I am not sure this is that easy. Suggestions more than welcome! Footnotes
|
|
In particular I wonder about the tradeoff compared to the Stack Overflow suggestion in https://stackoverflow.com/a/76754772. |
|
@lesteve I'll hopefully have some time this week to give a test - Thanks! |
That would be super useful, thanks 🙏 Just to give more context, maybe it would take me roughly one hour (rough estimate) to test it, think about brittleness of the code, look at ipykernel code to see how much of it has changed historically, and decide whether to merge it or not. The issue is that, as many other open-source maintainers, I have way many 1 hour tasks in my todo list than what I have time for. If I get a signal that someone cares enough about this problem to actually test the fix locally, this is good signal that working on getting this PR merged is worth it. On the other side of the spectrum you have comments like "any updates on this", "why hasn't this been merged yet, it's been more than 3 months/1 year/3 years", etc ... tend to be motivation killers for maintainers. |
An alternative way, using this PR is to make a new memory object pointed into the cache for each notebook Notebooks with the same function will clobber each otherwise. This also encourages DRY. I can test this weekend. |
|
@lesteve ok! I've given it a spin in a jupyter notebook inside vscode, and it now works as I expect. Please note, I have not tested it extensively, as unfortunately I'm also very pushed for time currently. But, its looking good to me! |
while joblib previously correctly identified that part of the dynamically generated folder name for functions defined in a notebook depended on the pid and should be cut, it has missed that the filename was dynamically generated too and depended on the murmur2 hash of the function code.
this has lead to pollution of memory namespace on each function change. instead, we collapse the ipykernel namespace to a single static part and let joblib.Memory handle the invalidation on code changes as it's supposed to do.
you can learn more here https://github.com/ipython/ipykernel/blob/main/ipykernel/compiler.py#L90