Skip to content

Conversation

@davidhassell
Copy link
Collaborator

Fixes #902

Should be considered after NCAS-CMS/cfdm#362 has been dealt with

@davidhassell davidhassell added this to the NEXTVERSION milestone Nov 1, 2025
@davidhassell davidhassell added enhancement New feature or request performance Relating to speed and memory performance labels Nov 1, 2025
Copy link
Member

@sadielbartholomew sadielbartholomew left a 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 | cf

so 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/recipes was due to linting and notably isort. 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. 😆)

@davidhassell
Copy link
Collaborator Author

I imagine the changes to anything in docs/source/recipes was due to linting and notably isort. Would you mind reverting those?

Yes - it was indeed linting. I can certainly revert the changes for this PR, but what to do in general (assuming that it's going to happen again when isort is run ...)?

@davidhassell
Copy link
Collaborator Author

davidhassell commented Nov 12, 2025

On my laptop, I get 1.3 seconds for the original, 0.75 seconds with improved cfdm, and 0.25 seconds with improved cfdm and improved cf-python. This is expected, because the the Dask import in cf-python would take ~0.5 seconds, independently of cfdm. Not sure what why our results are different ...?

davidhassell and others added 5 commits November 12, 2025 09:36
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
Co-authored-by: Sadie L. Bartholomew <sadie.bartholomew@ncas.ac.uk>
@davidhassell
Copy link
Collaborator Author

Reverted import orders in recipes: 727bb79

@davidhassell davidhassell merged commit 76c98f3 into NCAS-CMS:main Nov 12, 2025
@davidhassell davidhassell deleted the import-speed-up branch November 12, 2025 10:01
@sadielbartholomew
Copy link
Member

Yes - it was indeed linting. I can certainly revert the changes for this PR, but what to do in general (assuming that it's going to happen again when isort is run ...)?

There is a way to add a comment flag for isort to ignore those imports, but we don't want those to clog up the recipes in terms of what is rendered for the viewer. I guess the best way is to ignore the whole directory - I'll configure that when I update the pre-commiting and CI jobs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request performance Relating to speed and memory performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce the time taken to import cf

2 participants