ACT icon indicating copy to clipboard operation
ACT copied to clipboard

ENH: Use lazy loading for loading in modules

Open mgrover1 opened this issue 3 years ago • 13 comments
trafficstars

Currently, we eagerly import every module upon importing ACT (discovery, qc, etc.).

This means that when you call the

import act

line, it will run through all of the ACT code to prepare to use. This can lead to long import times, on the order of ~10 seconds.

We can cut this down quite a bit by using the SciPy SPEC 001, which outlines how to use the Lazy Loader package.

The only caveat here is that "Python 3.7, with PEP 562, introduces the ability to override module getattr and dir. In combination, these features make it possible to again provide access to submodules, but without incurring performance penalties."

Which indicated this will only work with Python 3.7+

I included a first cut of this within the PR, and removed our 3.6 linux test since the only way we can merge this is by losing that test.

Closes #62

mgrover1 avatar Jun 08 '22 15:06 mgrover1

@kenkehoe this might be what you've been looking for in terms of import speed/improvement

AdamTheisen avatar Jun 08 '22 16:06 AdamTheisen

@mgrover1 Looks like coverage dropped too, maybe due to imports? colormap module etc

zssherman avatar Oct 05 '22 21:10 zssherman

@mgrover1 Looks like coverage dropped too, maybe due to imports? colormap module etc

Yup - the coverage checks all the lines that are run. Since we are not running everything upon importing, we will need to add additional tests to cover everything (ex. colormap)

mgrover1 avatar Oct 05 '22 21:10 mgrover1

@zssherman similar to the xarray module, we do need to eagerly load in the colormaps for matplotlib

mgrover1 avatar Oct 05 '22 22:10 mgrover1

@zssherman these tests won't pass until it has the associated keys, which are only accessible when running on main (after the PR is merged)

mgrover1 avatar Oct 07 '22 15:10 mgrover1

@mgrover1 gotcha, do we wait until we figure out the python 3.6 ADC stuff?

zssherman avatar Oct 07 '22 16:10 zssherman

Probably - that makes sense!

mgrover1 avatar Oct 07 '22 16:10 mgrover1

@mgrover1 @zssherman generally, the coverage has been about 1.5% less so there are still some areas where we are seeing a decrease in the coverage. Let me look into it a little more on coveralls and see what's happening

AdamTheisen avatar Oct 07 '22 18:10 AdamTheisen

Pinging @kenkehoe here as well for awareness.

AdamTheisen avatar Oct 07 '22 18:10 AdamTheisen

@AdamTheisen I'm not really sure. How does this change the use of the read_netcdf() function use?

kenkehoe avatar Oct 07 '22 20:10 kenkehoe

@mgrover1 could you respond to Ken on this?

AdamTheisen avatar Oct 07 '22 20:10 AdamTheisen

@AdamTheisen I'm not really sure. How does this change the use of the read_netcdf() function use?

Users can still use the same API

import act

act.io.read_netcdf()

@kenkehoe Is this what you were asking?

mgrover1 avatar Oct 07 '22 21:10 mgrover1

So, coveralls is saying there's a decrease in other modules but when I drill down into them there's no change. I think the biggest change is that the following are not being loaded up at all initially here which is dropping the coverage. As @mgrover1 noted, this should be resolved when the tests run on production. I do think we should get an ENG in with ARM before we merge this to make sure there are no larger issues to think about.

Screenshot (3)

AdamTheisen avatar Oct 09 '22 14:10 AdamTheisen