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

ENH: use lazy loading #11838

Merged
merged 43 commits into from
Aug 24, 2023
Merged

ENH: use lazy loading #11838

merged 43 commits into from
Aug 24, 2023

Conversation

drammock
Copy link
Member

@drammock drammock commented Jul 27, 2023

Proof of concept for adopting SPEC 1.

~380 ms on main:

$ python -X importtime -c "import mne" 2>&1 | tail -n 1
import time:       950 |     381023 | mne

~205 ms on this PR after only converting mne/__init__.py (i.e. all the other __init__ files unchanged):

$ python -X importtime -c "import mne" 2>&1 | tail -n 1
import time:       841 |     206495 | mne

Todo:

  • Add CI run that uses EAGER_IMPORT=true env var to smoke out circular import issues
  • Convert all __init__.py
  • Wait for MAINT: Move underscores #11914
  • Get CIs green again (esp. EAGER_IMPORT=true)
  • Reconvert mne/io/__init__.py and convert mne/fiff/__init__.py

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Looks pretty straightforward and stays readable, nice!

@agramfort
Copy link
Member

so basically a 2x difference?

@drammock did you sort out the issue of scipy module if we don't nest the imports?

@larsoner
Copy link
Member

larsoner commented Aug 1, 2023

@drammock did you sort out the issue of scipy module if we don't nest the imports?

I'm not totally sure what this refers to... but my expectation is that this lazy_loader will make it so that we can un-nest a lot of imports like from scipy.linalg import svd and the like. They'll only get imported once you mne.minimum_norm.make_inverse_operator or whatever submodule you access actually needs/uses them.

@drammock
Copy link
Member Author

drammock commented Aug 1, 2023

That's what I expect too @larsoner. I got partway through the unnesting work and will finish it tomorrow and push the results

mne/_ola.py Outdated Show resolved Hide resolved
@larsoner larsoner changed the title RFC: use lazy loading ENH: use lazy loading Aug 10, 2023
@larsoner
Copy link
Member

IIUC the lazy_loader module will soon be used by NumPy and SciPy the same way we use it here. @agramfort this should make it acceptable for us to add it as a dependency from a clearing-legal-hurdles-for-managed-environments perspective right?

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

@drammock I think this one is finally ready!

@larsoner larsoner marked this pull request as ready for review August 23, 2023 17:59
mne/_fiff/__init__.py Outdated Show resolved Hide resolved
mne/channels/montage.py Show resolved Hide resolved
mne/evoked.py Show resolved Hide resolved
mne/tests/test_import_nesting.py Outdated Show resolved Hide resolved
Comment on lines 1 to 3
# # # WARNING # # #
# This list must also be updated in doc/_templates/autosummary/class.rst if it
# is changed here!
Copy link
Member Author

Choose a reason for hiding this comment

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

isn't this comment now cruft?

mne/utils/docs.py Show resolved Hide resolved
mne/_fiff/meas_info.py Outdated Show resolved Hide resolved
@larsoner
Copy link
Member

Thanks @drammock I'll push a commit with those fixes soon

@drammock drammock enabled auto-merge (squash) August 23, 2023 21:51
@drammock drammock merged commit dea8f87 into mne-tools:main Aug 24, 2023
@drammock drammock deleted the lazy-loading branch September 13, 2023 21:40
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
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.

3 participants