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

one possible fix for get_func_name in jupyter #1430

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

agramfort
Copy link
Contributor

this is one possible solution to #1312

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.10% ⚠️

Comparison is base (0c57c18) 94.82% compared to head (5a73e15) 94.72%.

❗ Current head 5a73e15 differs from pull request most recent head ff18375. Consider uploading reports for the commit ff18375 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1430      +/-   ##
==========================================
- Coverage   94.82%   94.72%   -0.10%     
==========================================
  Files          45       44       -1     
  Lines        7491     7203     -288     
==========================================
- Hits         7103     6823     -280     
+ Misses        388      380       -8     
Files Changed Coverage Δ
joblib/func_inspect.py 92.89% <100.00%> (+0.46%) ⬆️

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Could you please add an entry to the changelog?

I think we should also update the docstring of Memory.cache (or Memory.__init__) to document the possibility of spurious collisions for interactively functions defined with the same name in notebooks running in the same folder.

Ideally we should write some integration tests to check that the cache is reused across notebook kernel restart and simple edits of a notebook cell that does not change the function code itself but this is a lot of tedious work to get it up so I wouldn't make it a requirement to merge this PR.

joblib/func_inspect.py Outdated Show resolved Hide resolved
@agramfort
Copy link
Contributor Author

let me know what you think @ogrisel

the test requires pytest-requires and pytest_notebook packages.

@agramfort
Copy link
Contributor Author

@ogrisel @tomMoral I did what I could here.

Feel free to look at the CI failures. It seems to be issues with jupyter kernels not available on CIs:

https://dev.azure.com/joblib/joblib/_build/results?buildId=3475&view=logs&j=b4dccb76-6691-533e-59d0-802d21141eba&t=a3af8387-bc90-5fa8-47ce-fb6713046791

I did not look if/how jupyter is installed on CIs here.

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

Successfully merging this pull request may close these issues.

None yet

2 participants