Add support for handling zarr urls#7113
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7113 +/- ##
==========================================
+ Coverage 90.18% 90.20% +0.02%
==========================================
Files 91 91
Lines 25102 25160 +58
Branches 4706 4718 +12
==========================================
+ Hits 22638 22696 +58
Misses 1685 1685
Partials 779 779 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Some NotesSince @pp-mo @stephenworsley discussed this yesterday, here's my take on some things we found...
|
There was a problem hiding this comment.
Templating
This PR includes changes that may be worth sharing via templating. For each file listed below, please either:
- Action the suggestion via a pull request editing/adding the relevant file in the SciTools/.github
templates/directory. 1 - Raise an issue against the SciTools/.github repo for the above action if you really don't have 10mins spare right now. Include an assignee, to avoid it being forgotten.
- Dismiss the suggestion if the changes are not suitable for templating.
You will need to dismiss this review before this PR can be merged. Recommend the reviewer does this as their final action before merging, as this text will continually update as commits come in.
Template candidates
The following changed files are not currently templated, but their parent directories suggest they may be good candidates for a new template to be created:
Footnotes
-
Include this text in the PR body to avoid any notifications about applying the template changes back to the source repo!
@scitools-templating: please no update notification on: iris↩
ukmo-ccbunney
left a comment
There was a problem hiding this comment.
Looking good so far. Just a few suggestions and comments.
ukmo-ccbunney
left a comment
There was a problem hiding this comment.
Just a thought I meant to add to the original review...
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
ukmo-ccbunney
left a comment
There was a problem hiding this comment.
OK. Everything mostly looks good. I've made a few suggested changes to fix the failing test.
I've also pushed up some whatsnew fragments that Pak Lam and myself put together, see 90236d2
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
Co-authored-by: Chris Bunney <48915820+ukmo-ccbunney@users.noreply.github.com>
ukmo-ccbunney
left a comment
There was a problem hiding this comment.
Spotted a slight oddity...
| assert (output_path / ".zattrs").is_file() | ||
| assert (output_path / ".zgroup").is_file() | ||
|
|
||
| loaded = iris.load(output_uri)[0] |
There was a problem hiding this comment.
I've noticed that this zarr file isn't loading into a single cube.
If you load this via iris.load_cube() it fails to merge into a single cube.
Testing on my VDI shows that it actually loads into two cubes - the second being the surface_altitude cube.
The equality test still passes (as it takes the first cube via slice [0]), so surface_altitude is getting added as an AuxCoord on the first cube ok, so it is a bit odd.
Furthermore, when accessing the data on my VDI, I get the following errors thrown from the HDF5 layer, although they don't actually stop me from loading the data (so more warnings). I don't see the same warnings in the CI tests though (I'm running the same version of netcdf)
HDF5-DIAG: Error detected in HDF5 (1.14.6) thread 1:
#000: H5F.c line 496 in H5Fis_accessible(): unable to determine if file is accessible as HDF5
major: File accessibility
minor: Not an HDF5 file
#001: H5VLcallback.c line 3913 in H5VL_file_specific(): file specific failed
major: Virtual Object Layer
minor: Can't operate on object
#002: H5VLcallback.c line 3848 in H5VL__file_specific(): file specific failed
major: Virtual Object Layer
minor: Can't operate on object
#003: H5VLnative_file.c line 344 in H5VL__native_file_specific(): error in HDF5 file check
major: File accessibility
minor: Can't get value
#004: H5Fint.c line 1055 in H5F__is_hdf5(): unable to open file
major: File accessibility
minor: Unable to initialize object
#005: H5FD.c line 787 in H5FD_open(): can't open file
major: Virtual File Layer
minor: Unable to open file
#006: H5FDsec2.c line 323 in H5FD__sec2_open(): unable to open file: name = 'file:///tmp/4dwe.zarr#mode=nczarr', errno = 2, error message = 'No such file or directory', flags = 0, o_flags = 0
major: File accessibility
minor: Unable to open file
There was a problem hiding this comment.
So, after some investigation, it has become apparent that the realistic_4d_w_everything cube doesn't round trip if you save and load it as a netcdf file either - the same merge errors apply.
The same applied with iris.COMBINE_POLICY.set("comprehensive")
So this is not a NcZarr loading issue.
I can't immediately see why the file doesn't round trip, but it is clearly no the fault of the nczarr loading, so we can ignore that in the context of this PR.
The issue of the HDF warnings still applies, but this is just noise, rather than an error (the data is still read correctly) and it is not showing in the CI tests - only locally for me. I would be interested if @trexfeathers sees similar noise when running locally?
Other than the HDF5 noise, I am happy with the the implementation of the NcZarr functionality.
There was a problem hiding this comment.
After a bit of poking, I've narrowed down the HDF noise to the netCDF4.Dataset call in _ThreadSafeWrapper.__getitem__ where the dataset is repeatedly opened/closed.
Furthermore, it is only a problem when dask is used - if i force the data to be loaded as concrete data (by upping the minimum data size threshold) then there are no HDF warnings on the call the netCDF4.Dataset. Assumedly this has something to do with dask making lots of concurrent calls to op the dataset due to multiple chunks trying to read from it?
If i set dask.config.set('scheduler', 'single-threaded') the error also goes away, which gives further evidence that it is a dask concurrency issue.
At the moment, I don't know WHY it is a problem for NcZarr and OK for NetCDF...
There was a problem hiding this comment.
Right - I've got Copilot on board now to help me with the dask interaction.
It has suggested the following test that nicely proves that the HDF5 noise issue is actually not related to dask or Iris, but is generated when any worker-thread makes a call to netcdf4.Dataset with a NCZarr URI:
>>> from concurrent.futures import ThreadPoolExecutor
>>> import netCDF4
>>> uri = "file:///tmp/4dwe.zarr#mode=nczarr,file"
>>> def f():
>>> ds = netCDF4.Dataset(uri)
>>> ds.close()
>>> f() # main thread
>>> with ThreadPoolExecutor(max_workers=1) as ex:
>>> ex.submit(f).result() # worker thread
HDF5-DIAG: Error detected in HDF5 (1.14.6) thread 3:
#000: H5F.c line 496 in H5Fis_accessible(): unable to determine if file is accessible as HDF5
major: File accessibility
minor: Not an HDF5 fileAs the tests pass, I think we can consider this just slightly annoying "noise" as a result of Dask accessing NCZarr datasets via it's worker thread(s).
If necessary, to quieten the warnings we could recommend either:
- filtering the HDF5 warnings
- Using the
single-threadeddask scheduler (although potential performance impact?)
Closes #6967