-
Notifications
You must be signed in to change notification settings - Fork 23
Reduce the time taken to import cf
#905
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
base: main
Are you sure you want to change the base?
Conversation
sadielbartholomew
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.
Hmm, the code changes all look fine and sensible for improving import time to some extent, but as for timings on my local system, I am not seeing much speed-up on this branch relative to main in terms of import time, at least judging it by the useful test command you shared in one of the original issues.
There is a bit, which is of course good, but nothing like the factor 6 you saw when you tested as per your comment on the Issue. For example, on first run in a fresh terminal tab, with the cfdm branch kept on the one just merged with its own import timing improvements:
$ git checkout main # after
...
import time: 1297 | 606844 | cf
$ git checkout david/import-speed-up # after
...
import time: 1160 | 602408 | cfso remains ~0.6 s, only 1% faster.
I wonder if you are quoting the factor 6 improvement using timings with this branch in combination with the cfdm sister one NCAS-CMS/cfdm#362, relative to the cfdm main (before that was merged) - i.e. the timing improvement is mostly down to the cfdm-side changes? I do notice considerable speed-up on the cf-python side with the cfdm branch set to that relative to main before: 0.6 s relative to 1.3 s. But we can't attribute that to this PR.
So, unless I'm missing something this cf-side PR doesn't seem to produce that significant of a reduction in import time, but it does produce a small one which is good enough to be merge-worthy - so I am approving (but would be interesting to know more context to your own timings). Please merge after checking the small number of minor feedback items raised, plus this more general comment:
- I would prefer it if we don't change the import order in the recipes, since those were carefully placed to avoid the possibility for seg faulting due to bad matplotlib and esmpy lower-level code interactions (see NCAS-CMS/cf-plot#24) - however I will review that issue in terms of whether it can be avoided given the state of dependencies on our and those libraries now. I imagine the changes to anything in
docs/source/recipeswas due to linting and notablyisort. Would you mind reverting those?
(Again it is a shame we can't use 'PEP 810 – Explicit lazy imports' now to avoid the movement of imports as per my notes on the cfdm PR. But should be good for the next several years until Python 3.15 is the standard minimum. 😆)
| **2025-12-??** | ||
|
|
||
| * Reduce the time taken to import `cf` | ||
| (https://github.com/NCAS-CMS/cfdm/issues/902) |
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.
| (https://github.com/NCAS-CMS/cfdm/issues/902) | |
| (https://github.com/NCAS-CMS/cf-python/issues/902) |
| # Define some data values | ||
| _data = {} |
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.
Ideally either the variable name (preferred) or comment here can be more descriptive e.g. changed to datetime_data or units_data, as neither says anything as-is.
| # regridding = _get_module_info("esmpy", alternative_name="ESMF", try_except=True) | ||
|
|
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.
Left over during dev?
| # regridding = _get_module_info("esmpy", alternative_name="ESMF", try_except=True) |
|
|
||
| _minimum_vn = "3.10.0" | ||
| if Version(python_version()) < Version(_minimum_vn): | ||
| # Check the version of cfdm (this is worth doing because of the very |
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.
As for the other checks, same note as NCAS-CMS/cfdm#362 (comment) so might be worth noting on the related Issue you opened, NCAS-CMS/cfdm#364, to consider cf-python as well.
Fixes #902
Should be considered after NCAS-CMS/cfdm#362 has been dealt with