Handle the renaming of ESMF Python module #639

sadielbartholomew · 2023-04-26T08:51:26Z

Close #633. Note the following:

As discussed, we are allowing either esmpy (new name) or esmf (old) to be imported for backwards compatibility, for now, as advertised on the new change log entry.
As part of this, to cater for that, I updated the internal _get_module_info function to cater for a possible alternative module name, since that might occur for other modules, but even if not it allows that method to be re-used for this case as it was before. Now cf.environment picks up the module under either name, so is listed as esmpy/ESMF rather than ESMF in the output dict.
All ESMF_* methods (e.g. ) have become esmpy_* methods (now esmpy_regrid_1d), i.e. the name conversion has also been made on method names. This makes sense to me, because the capitalised old name was defying naming conventions for methods and was ugly and weird in that way, plus now we use the newer name going forward. But please do confirm that this is OK, since it is an API change. (ON a related note, very few of those methods seem to be documented - is that something that needs addressing?)

Other issues flagged

I don't think this is related to this PR, but I need to investigate further... test_Field_regrid_2d_field is currently hanging for me locally (I can't test this on main without downgrading my ESMPy and conda hates downgrading anything so it will be a bit tricky to check quickly...). With the PR, all other test_regrid.py methods pass and in fact the whole test suite passes other than that when I skip it to stop to hanging, indicating things are fine otherwise.

Please let me know if you see this too! I suspect it could be to do with the newer ESMPy version and I will bump this to a new issue if we see further evidence of that!

Specifically, it hangs (I waited 5 mins or so before assuming that) so I apply:

diff --git a/cf/test/test_regrid.py b/cf/test/test_regrid.py
index ec4c9a4a0..34317cd7e 100644
--- a/cf/test/test_regrid.py
+++ b/cf/test/test_regrid.py
@@ -140,7 +140,8 @@ class RegridTest(unittest.TestCase):
     dst = dst_src[0]
     src = dst_src[1]
 
-    @unittest.skipUnless(esmpy_imported, "Requires esmpy/ESMF package.")
+    @unittest.skipIf(True, "HANGING BUT WHY?")
+    # Unless(esmpy_imported, "Requires esmpy/ESMF package.")
     def test_Field_regrid_2d_field(self):
         """2-d regridding with Field destination grid."""
         self.assertFalse(cf.regrid_logging())

to skip the hanging test and everything passes:

$ py test_regrid.py 
Run date: 2023-04-26 09:49:42.736762
Platform: Linux-4.15.0-54-generic-x86_64-with-glibc2.10 
HDF5 library: 1.14.0 
netcdf library: 4.9.2 
udunits2 library: /home/sadie/anaconda3/envs/cf-env/lib/libudunits2.so.0 
esmpy/ESMF: 8.4.1 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/esmpy/__init__.py
Python: 3.8.16 /home/sadie/anaconda3/envs/cf-env/bin/python
dask: 2023.1.0 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/dask/__init__.py
netCDF4: 1.6.3 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/netCDF4/__init__.py
psutil: 5.9.4 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/psutil/__init__.py
packaging: 23.0 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/packaging/__init__.py
numpy: 1.24.2 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/numpy/__init__.py
scipy: 1.10.1 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/scipy/__init__.py
matplotlib: 3.4.3 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/matplotlib/__init__.py
cftime: 1.6.0 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/cftime/__init__.py
cfunits: 3.3.5 /home/sadie/cfunits/cfunits/__init__.py
cfplot: 3.1.18 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/cfplot/__init__.py
cfdm: 1.10.1.0 /home/sadie/cfdm/cfdm/__init__.py
cf: 3.14.1 /home/sadie/cf-python/cf/__init__.py

test_Field_regrid_2d_field (__main__.RegridTest)
2-d regridding with Field destination grid. ... skipped 'HANGING BUT WHY?'
test_Field_regridc_1d_field (__main__.RegridTest)
1-d Cartesian regridding with Field destination grid. ... ok
test_Field_regridc_2d_coords (__main__.RegridTest)
2-d Cartesian regridding with coords destination grid. ... ok
test_Field_regridc_3d_field (__main__.RegridTest)
3-d Cartesian regridding with Field destination grid. ... ok
test_Field_regridc_domain (__main__.RegridTest)
Spherical regridding with Domain destination grid. ... ok
test_Field_regrids_bad_dst (__main__.RegridTest)
Disallowed destination grid types raise an exception. ... ok
test_Field_regrids_coords (__main__.RegridTest)
Spherical regridding with coords destination grid. ... ok
test_Field_regrids_domain (__main__.RegridTest)
Spherical regridding with Domain destination grid. ... ok
test_Field_regrids_field_operator (__main__.RegridTest)
Spherical regridding with operator destination grid. ... ok
test_Field_regrids_non_coordinates (__main__.RegridTest)
Check setting of non-coordinate metadata. ... ok

----------------------------------------------------------------------
Ran 10 tests in 26.558s

OK (skipped=1)

sadielbartholomew · 2023-04-26T08:58:24Z

The CI test suite jobs are failing on setup, namely package dependency conflicts RE cf and cfdm (cf-python 3.14.1 requires cfdm<1.10.1.0,>=1.10.0.3, but you have cfdm 1.10.1.0 which is incompatible.) so can be ignored, those it might be useful to see those later so I could bodge the constraints if need be. I'll await your review first, David.

davidhassell

Hi Sadie - very thorough job. Thanks. I have tested the new code with both 8.2.0 and 8.4.0, and both work just fine. I have one minor question about nested try/except clauses, which you should resolve in anyway you like (including ignoring it).

Please merge whenever you're ready.

cf/regrid/regrid.py

Changelog.rst

sadielbartholomew · 2023-04-26T12:36:53Z

@davidhassell thanks for the review. What do you see with regards to my comment under 'Other issues flagged' in #639 (comment)?

Co-authored-by: David Hassell <[email protected]>

davidhassell · 2023-04-26T13:15:12Z

Hi Sadie - I've run test_regrid.py with ESMF versions 8.2.0 and 8.4.0 on my system, and both run fine, taking ~90 seconds and ~
~80 seconds respectively.

sadielbartholomew · 2023-04-26T13:24:54Z

Thanks David. Indeed, I was not being patient enough, having not noticed that that one test method takes quite a while relative to the others. Hence assuming it was hanging. I just got:

$ time py test_regrid.py 
Run date: 2023-04-26 14:20:19.667060
Platform: Linux-4.15.0-54-generic-x86_64-with-glibc2.10 
HDF5 library: 1.14.0 
netcdf library: 4.9.2 
udunits2 library: /home/sadie/anaconda3/envs/cf-env/lib/libudunits2.so.0 
esmpy/ESMF: 8.4.1 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/esmpy/__init__.py
Python: 3.8.16 /home/sadie/anaconda3/envs/cf-env/bin/python
dask: 2023.1.0 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/dask/__init__.py
netCDF4: 1.6.3 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/netCDF4/__init__.py
psutil: 5.9.4 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/psutil/__init__.py
packaging: 23.0 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/packaging/__init__.py
numpy: 1.24.2 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/numpy/__init__.py
scipy: 1.10.1 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/scipy/__init__.py
matplotlib: 3.4.3 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/matplotlib/__init__.py
cftime: 1.6.0 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/cftime/__init__.py
cfunits: 3.3.5 /home/sadie/cfunits/cfunits/__init__.py
cfplot: 3.1.18 /home/sadie/anaconda3/envs/cf-env/lib/python3.8/site-packages/cfplot/__init__.py
cfdm: 1.10.1.0 /home/sadie/cfdm/cfdm/__init__.py
cf: 3.14.1 /home/sadie/cf-python/cf/__init__.py

test_Field_regrid_2d_field (__main__.RegridTest)
2-d regridding with Field destination grid. ... ok
test_Field_regridc_1d_field (__main__.RegridTest)
1-d Cartesian regridding with Field destination grid. ... ok
test_Field_regridc_2d_coords (__main__.RegridTest)
2-d Cartesian regridding with coords destination grid. ... ok
test_Field_regridc_3d_field (__main__.RegridTest)
3-d Cartesian regridding with Field destination grid. ... ok
test_Field_regridc_domain (__main__.RegridTest)
Spherical regridding with Domain destination grid. ... ok
test_Field_regrids_bad_dst (__main__.RegridTest)
Disallowed destination grid types raise an exception. ... ok
test_Field_regrids_coords (__main__.RegridTest)
Spherical regridding with coords destination grid. ... ok
test_Field_regrids_domain (__main__.RegridTest)
Spherical regridding with Domain destination grid. ... ok
test_Field_regrids_field_operator (__main__.RegridTest)
Spherical regridding with operator destination grid. ... ok
test_Field_regrids_non_coordinates (__main__.RegridTest)
Check setting of non-coordinate metadata. ... ok

----------------------------------------------------------------------
Ran 10 tests in 109.699s

OK

real	1m51.548s
user	1m52.335s
sys	0m2.182s

So that was a non-issue in the end. Thanks for your help there. All good.

I'll respond to your feedback and let you know once that's done. Should be 15 minutes or so. Then I think we're ready to merge unless you feel otherwise.

davidhassell · 2023-04-26T13:28:23Z

All good!

davidhassell · 2023-04-26T13:35:23Z

Hang on! docs/source/installation.rst needs updating too:

conda install -c conda-forge "esmpy>=8.0.0"

ought to do it, I think.

davidhassell · 2023-04-26T13:36:35Z

... and the conda command appears twice in that file.

sadielbartholomew · 2023-04-26T14:03:15Z

Hang on! docs/source/installation.rst needs updating too

Oh, another good spot. One moment, I'll add a further commit. For now note I've resolved, I believe, the try/except nest conversion.

sadielbartholomew · 2023-04-26T14:30:04Z

Hi David, I should have addressed everything you have raised, now, so ready for re-review. Thanks.

davidhassell

Great. I've re-tested with both versions, and everything works. Good to go, I think.

sadielbartholomew added 9 commits April 25, 2023 18:03

Import ESMF/esmpy via new & old module names in regrid module

41d6b64

Catch both old and new ESMF/esmpy name in init script

7e83afe

Update functions module to cater for alternative module names

4dc59c7

Convert references to ESMF to 'esmpy' throughout rest of codebase

d815e6a

Update test_regrid to cater for alternative ESMF module names

82e7e0a

Fix catch for lack of ESMF/esmpy module under either name

82eba1c

Self review: reinstate a few legitimate references to 'ESMF'

67afb3e

Self review: refer to alternate name in test skip message

b6018f4

Add changelog entry to cover ESMF Python module name change

52d2ffb

sadielbartholomew added installation Rrelating to installing the library regridding Relating to regridding operations labels Apr 26, 2023

sadielbartholomew added this to the 3.15.0 milestone Apr 26, 2023

sadielbartholomew requested a review from davidhassell April 26, 2023 08:51

davidhassell approved these changes Apr 26, 2023

View reviewed changes

cf/regrid/regrid.py Outdated Show resolved Hide resolved

davidhassell requested changes Apr 26, 2023

View reviewed changes

Changelog.rst Show resolved Hide resolved

Update Changelog.rst to include esmpy dependency constraint

95ec822

Co-authored-by: David Hassell <[email protected]>

Address feedback: prefer nested try/except over alternative

3324885

Update docs for esmpy module name change

506683b

davidhassell approved these changes Apr 26, 2023

View reviewed changes

sadielbartholomew merged commit 951f5be into NCAS-CMS:main Apr 26, 2023

sadielbartholomew deleted the esmf-py-module-renaming branch April 26, 2023 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle the renaming of ESMF Python module #639

Handle the renaming of ESMF Python module #639

sadielbartholomew commented Apr 26, 2023 •

edited

Loading

sadielbartholomew commented Apr 26, 2023

davidhassell left a comment

sadielbartholomew commented Apr 26, 2023

davidhassell commented Apr 26, 2023

sadielbartholomew commented Apr 26, 2023

davidhassell commented Apr 26, 2023

davidhassell commented Apr 26, 2023

davidhassell commented Apr 26, 2023

sadielbartholomew commented Apr 26, 2023

sadielbartholomew commented Apr 26, 2023

davidhassell left a comment

Handle the renaming of ESMF Python module #639

Handle the renaming of ESMF Python module #639

Conversation

sadielbartholomew commented Apr 26, 2023 • edited Loading

Other issues flagged

sadielbartholomew commented Apr 26, 2023

davidhassell left a comment

Choose a reason for hiding this comment

sadielbartholomew commented Apr 26, 2023

davidhassell commented Apr 26, 2023

sadielbartholomew commented Apr 26, 2023

davidhassell commented Apr 26, 2023

davidhassell commented Apr 26, 2023

davidhassell commented Apr 26, 2023

sadielbartholomew commented Apr 26, 2023

sadielbartholomew commented Apr 26, 2023

davidhassell left a comment

Choose a reason for hiding this comment

sadielbartholomew commented Apr 26, 2023 •

edited

Loading