-
Notifications
You must be signed in to change notification settings - Fork 339
Fixed #495 Reduce Import Time (Lazy Imports) #1122
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
Conversation
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1122 |
|
@NimaSarajpoor Please review at your earliest convenience. Apologies for changes in so many files! I realized how inconsistently the unit tests were importing modules (i.e., Originally, I had noticed that the import time was extremely high immediately after |
Great improvement!!
Will review it within the next couple of days. |
|
I should note that I noticed an issue with lazy imports (this PR) that didn't exist with eager imports (in Case One:
This should print "<function stump at 0x127286b90>" and thus showing that it is a function. However Case Two:
This should print "<module 'stumpy.stump' from '/Users/slaw/Git/stumpy-dev.git/stumpy/stump.py'>" and thus showing that it is a module (not a function). So, somehow, tab-completion causes things to ignore lazy loading and possibly revert/fall back to disambiguating to a module. In the original ( and this instantly makes |
This has been fixed in the latest commit 2db3a58 Next, I'll need to run the test suite on Google Colab to ensure that CUDA related things aren't broken |
I can confirm that all tests are passing on Google Colab! The key was making sure to update the pre-installed Produces: |
NimaSarajpoor
left a comment
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.
@seanlaw
Left a few comments for your considerations. If you notice a comment does not make sense, please let me know!! I tried to understand the logic implemented in __init__.py. Hopefully I did not miss anything...
Thanks @NimaSarajpoor. I've made all of the suggested changes |
|
Thanks for addressing the comments. No more comments from my side. Since you already tested it in Google Colab, all should be good. Please let me know if you want me to pay extra attention to a particular part. Otherwise, looks good to be merged! Great work!! |
|
Thanks for the assist @NimaSarajpoor! |
See #495
Pull Request Checklist
Below is a simple checklist but please do not hesitate to ask for assistance!
black(i.e.,python -m pip install blackorconda install -c conda-forge black)flake8(i.e.,python -m pip install flake8orconda install -c conda-forge flake8)pytest-cov(i.e.,python -m pip install pytest-covorconda install -c conda-forge pytest-cov)black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./in the root stumpy directoryflake8 --extend-exclude=.venv ./in the root stumpy directory./setup.sh dev && ./test.shin the root stumpy directory