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

Lazily load data_dir into the top-level namespace #5996

Merged
merged 8 commits into from
Oct 29, 2021

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Oct 29, 2021

Description

This is a follow up PR to #5101 that delays loading of skimage.data_dir. Currently, importing that in the top-level __init__.py results in importing Pooch. With the change here, data_dir is lazily loaded.

The diff here is much smaller than it appears because all code in _fetchers.py is just the old __init__.py. Aside from removing __all__ after the rename, I made only a 1 line change to fix an issue with parsing the modified __version__ string containing the git hash introduced in #5909.

This PR also fixes an issue where skimage.data.vortex was missing from skimage.data.__all__.

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

The top-level import of data_dir in skimage/__init__.py accounted for
approximately 1/3 of the package import time. This PR removes this overhead.

This commit moves skimage/data/__init__.py to skimage/data/_fetchers.py
and then adds a new skimage/data/__init__.py that lazily loads from the
skimage.data submodules.
lazy.attach generates __all__ in data/__init__.py now
@grlee77 grlee77 added the 🔧 type: Maintenance Refactoring and maintenance of internals label Oct 29, 2021
@pep8speaks
Copy link

pep8speaks commented Oct 29, 2021

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 439:80: E501 line too long (98 > 79 characters)
Line 480:80: E501 line too long (81 > 79 characters)
Line 506:80: E501 line too long (578 > 79 characters)
Line 511:80: E501 line too long (84 > 79 characters)
Line 555:80: E501 line too long (98 > 79 characters)
Line 566:80: E501 line too long (81 > 79 characters)
Line 963:80: E501 line too long (99 > 79 characters)
Line 969:80: E501 line too long (80 > 79 characters)

Comment last updated at 2021-10-29 20:08:35 UTC

@grlee77 grlee77 requested a review from stefanv October 29, 2021 17:09
@grlee77
Copy link
Contributor Author

grlee77 commented Oct 29, 2021

This lazy loading should also help resolve #5146 (at least the error would not occur upon the top-level import). I will add another commit here to deal with the issue of missing pooch.__version__ on older Pooch (as reported in #5146 (comment))

@grlee77
Copy link
Contributor Author

grlee77 commented Oct 29, 2021

I manually verified that 26a580e avoids the problem with AttributeError: module 'pooch' has no attribute '__version__' with Pooch 1.1.0 since we do not run CI tests with that version.

@grlee77
Copy link
Contributor Author

grlee77 commented Oct 29, 2021

closes #5146

@alexdesiqueira alexdesiqueira merged commit c6535b3 into scikit-image:main Oct 29, 2021
@alexdesiqueira
Copy link
Member

Thank you @grlee77!

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/why-does-init-py-attempt-to-download-sample-data/79665/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants