[NO NOT MERGE before cfdm 1.9.1.0] dask: compression refactor

[davidhassell]

Refactor of representation of compressed arrays, building on NCAS-CMS/cfdm#167 and NCAS-CMS/cfdm#168

Merged in lama-to-dask on 2022-03-10

[davidhassell]

Hi Sadie - I'll be leaving this alone, now. I was just checking on some things that I forgotten about since writing this, and improved some of the comments. Thanks.

[sadielbartholomew]

Thanks David. I will try to do a final review for this PR this evening, if not by early next week.

[sadielbartholomew]

It's always great when many more lines are dropped than added - nice job!

These are just some comments from eyeballing the code; I am still reviewing the functionality. I'll try to submit the final review later today. Thanks.

[sadielbartholomew]

All good, including previously-raised feedback being fully addressed, except for one possible issue, namely that I see a lone test failure due to a test netCDF file not being found. I am not sure why that file isn't being picked up (I ran python create_test_files.py before running python test_Data.py, and interactively cfdm.read("subsampled_2.nc") works fine)?

======================================================================
ERROR: test_Data__init__compression (__main__.DataTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_Data.py", line 3827, in test_Data__init__compression
    f = cfdm.read("subsampled_2.nc")[-3]
  File "/home/sadie/cfdm/cfdm/read_write/read.py", line 310, in read
    raise IOError(f"Can't read non-existent file {filename}")
OSError: Can't read non-existent file subsampled_2.nc

----------------------------------------------------------------------
Ran 85 tests in 14.292s

FAILED (errors=1, skipped=41)

[davidhassell]

Sorry! da9b13b and 378d5ba should fix this.

[sadielbartholomew]

Regarding 378d5ba, I am not sure pip is happy with the beta marker, since I see:

$ pip install -e .
...
...
  Running setup.py develop for cfdm
ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
cf-python 4.0.0b0 requires cfdm<1.9.2.0,>=1.9.1.0, but you have cfdm 1.9.1.0b0 which is incompatible.
Successfully installed cfdm-1.9.1.0b0

(Note the lines above the final 'success' report, which indicate a compatibility issue.)

I can work around that, but when I do I am still seeing the same test failure on the missing file. Could the current merge conflicts have anything to do with it (something that needs to be in data/creation.py for this to work, say)?

[davidhassell]

Hi Sadie. Soory this has been a bit fiddly. Merge conflicts fixed. perhaps they could have affected anything ... I have just run python create_test_files.py with latest commits in, and it does create the required files.

I've changed the lower cfdm version to 1.9.0.1 for now - will that help?

[davidhassell]

With respect to the to_memory keyword to Data.__init__, I wonder if it should be called persist instead, and then we add a self.persist() call in ? E.g.

<snip Data.__init__>
         # Bring the data into memory
         if persist:
             array.persist()

         # Store the dask array
         self._set_dask(array, delete_source=False, reset_mask_hardness=False) 

         # Set the mask hardness on each chunk.
         self.hardmask = hardmask

        

[sadielbartholomew]

Hi @davidhassell, sorry I lost track of this one and notably missed your latest comments (mostly because GitHub's 'Review requests' list, which I use to monitor review tasks, drops any PR one you have left a review once already, but of course I can't blame that wholly since I have known it works like that for a long time!).

I'm just about to do a final sanity check after you have added the aforementioned updates, and will leave that as a final review shortly. As for:

With respect to the to_memory keyword to Data.init, I wonder if it should be called persist instead, and then we add a self.persist() call

I really like that idea, which will keep us consistent with Dask in that respect and in turn likely align us with any Python Array API guidance on terminology for such methods. Shall we wait until after the Dask migration work is complete, though, in line with our current tentative plan to do the migration and then make a batch of appropriate API changes to comprise the final 4.0.0 version?

[davidhassell]

Hi Sadie - thanks. Since Data.__init__ does not currently have a to_memory keyword, I don't see a problem in introducing a persist one. Is that OK? I'll put in a new commit, if so.

[sadielbartholomew]

Is that OK? I'll put in a new commit, if so.

Thanks, that sounds great so please go ahead. I'll wait until you push that commit to confirm the final review.

[davidhassell]

Great. Done.

[sadielbartholomew]

All previous feedback has been addressed well and the persist keyword has been implemented solidly. Notably, the one outstanding issue of:

I see a lone test failure due to a test netCDF file not being found
...
OSError: Can't read non-existent file subsampled_2.nc

has been fixed, so that I now get ​a full local pass of test_Data.

All good! Please merge.