Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object #3095

crusaderky · 2019-07-11T13:16:16Z

Closes REGRESSION: copy(deep=True) casts unicode indices to object #3094
Tests added
Fully documented, including whats-new.rst for all changes and api.rst for new API

codecov · 2019-07-11T13:20:30Z

Codecov Report

Merging #3095 into master will decrease coverage by 0.09%.
The diff coverage is 90%.

@@            Coverage Diff            @@
##           master    #3095     +/-   ##
=========================================
- Coverage   95.75%   95.65%   -0.1%     
=========================================
  Files          63       63             
  Lines       12804    12807      +3     
=========================================
- Hits        12260    12251      -9     
- Misses        544      556     +12

shoyer · 2019-07-12T16:54:14Z

xarray/core/indexing.py

        return ('%s(array=%r, dtype=%r)'
                % (type(self).__name__, self.array, self.dtype))
+
+    def copy(self, deep: bool = True) -> 'PandasIndexAdapter':
+        obj = object.__new__(PandasIndexAdapter)


Is it possible to use the constructor directly here rather than object.__new__?

If not, please add a comment explaining why :)

I can use __init__, but it's going to be needlessly slower. If you prefer I can add a fastpath=False parameter to __init__, which is the pattern already used elsewhere

I would rather stick with init unless we can find an example of using xarray's public APIs where performance is meaningfully effected. Until then it's probably premature optimization.

xarray/core/indexing.py

shoyer · 2019-07-15T04:47:27Z

xarray/core/indexing.py

+    def copy(self, deep: bool = True) -> 'PandasIndexAdapter':
+        # Not the same as just writing `self.array.copy(deep=deep)`, as
+        # shallow copies of the underlying numpy.ndarrays become deep ones
+        # upon pickling


I don't quite follow this comment -- how does pickling relate to this new method?

pandas.Index.copy(deep=False) creates new identical views of the underlying numpy arrays.
A numpy view becomes a real, deep-copied numpy array upon pickling. This is by design to prevent people from accidentally sending a huge numpy array over network or IPC when they just want to send a slice of it. Crucially though, this (IMHO improvable) design makes no distinction when the base array is the same size or smaller than the view.

Back to the method at hand: if two xarray.DataArray/Dataset share the same underlying pandas.Index, which is the whole intent of copy(deep=False), when they are pickled together they continue sharing the same data. This is already true when you shallow-copy xarray.Variable.
But if instead of

array = self.array.copy(deep=True) if deep else self.array

we wrote

array = self.array.copy(deep=deep)

which would be tempting as it is much more readable, everything would be the same initially, until you pickle the original index and its shallow copy together, at which point the copy would automatically and silently become a deep one, causing double RAM/disk usage upon storing/unpickling.

>>> idx = xarray.IndexVariable('x', list(range(500000)))._data.array >>> len(pickle.dumps((idx, idx))) 4000281 >>> len(pickle.dumps((idx, idx.copy(deep=False)))) 8000341

This was a real problem I faced a couple of years ago where I had to dump to disk (for forensic purposes) about 10,000 intermediate steps of a Monte Carlo simulation, each step being a DataArray or Dataset. The data variables were all dask-backed, so they were fine. But among the indices I had a 'scenario' dimension with 500,000 points, dtype='<U13'.

Before pickling: 13 * 500,000 = 6.2 MB; the 50,000 shallow copies of it being just views
After pickling: 13 * 500,000 * 10,000 = I needed a new hard disk!

This issue was from a couple of years ago and has since been fixed. My (apparently overcomplicated) code above prevents it from showing up again.

P.S. this, by the way, is a big problem with xarray.align when you are dealing with mixed numpy+dask arrays

>>> a = xarray.DataArray(da.ones(500000, chunks=10000), dims=['x']) >>> b = xarray.concat([a] + [xarray.DataArray(i) for i in range(1000)], dim='y') >>> c = b.sum() >>> c.compute() <xarray.DataArray ()> array(2.497505e+11) # Everything is fine so far (not very fast due to the dask chunks of size 1 but still)... >>> client = distributed.Client() >>> c.compute()

Watch the RAM usage of your dask client rocket to 8 GB, and several GBs worker-side too.
This will take a while because, client side, you just created 3.8 GB (500000 * 8 * 1000) worth of pickle file and are now sending it over the network.

[EDIT] nvm this last example. The initial concat() is already sending the RAM usage to 4 GB, meaning it's not calling numpy.ndarray.broadcast_to under the hood as it used to do...

Thanks for the explanation, this makes complete sense.

crusaderky · 2019-07-19T09:15:41Z

@shoyer is there anything outstanding on this?

crusaderky · 2019-07-24T09:20:17Z

@shoyer ping

crusaderky · 2019-07-31T16:21:04Z

@shoyer this has now been waiting for 16 days, is there anybody who could pick the review up in your absence?

shoyer

Sorry for delay here!

As you can tell, I've been pretty distracted from code reviews for xarray recently.

xarray/core/indexing.py

shoyer · 2019-08-02T04:42:35Z

xarray/core/indexing.py

+    def copy(self, deep: bool = True) -> 'PandasIndexAdapter':
+        # Not the same as just writing `self.array.copy(deep=deep)`, as
+        # shallow copies of the underlying numpy.ndarrays become deep ones
+        # upon pickling


Thanks for the explanation, this makes complete sense.

crusaderky · 2019-08-02T13:58:14Z

@shoyer ready for merge

max-sixty · 2019-08-02T14:02:56Z

Thanks @crusaderky !

* master: (68 commits) enable sphinx.ext.napoleon (pydata#3180) remove type annotations from autodoc method signatures (pydata#3179) Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object (pydata#3095) Fix distributed.Client.compute applied to DataArray (pydata#3173) More annotations in Dataset (pydata#3112) Hotfix for case of combining identical non-monotonic coords (pydata#3151) changed url for rasterio network test (pydata#3162) to_zarr(append_dim='dim0') doesn't need mode='a' (pydata#3123) BUG: fix+test groupby on empty DataArray raises StopIteration (pydata#3156) Temporarily remove pynio from py36 CI build (pydata#3157) missing 'about' field (pydata#3146) Fix h5py version printing (pydata#3145) Remove the matplotlib=3.0 constraint from py36.yml (pydata#3143) disable codecov comments (pydata#3140) Merge broadcast_like docstrings, analyze implementation problem (pydata#3130) Update whats-new for pydata#3125 and pydata#2334 (pydata#3135) Fix tests on big-endian systems (pydata#3125) XFAIL tests failing on ARM (pydata#2334) Add broadcast_like. (pydata#3086) Better docs and errors about expand_dims() view (pydata#3114) ...

* master: enable sphinx.ext.napoleon (pydata#3180) remove type annotations from autodoc method signatures (pydata#3179) Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object (pydata#3095) Fix distributed.Client.compute applied to DataArray (pydata#3173)

Guido Imperiale added 2 commits July 11, 2019 14:13

Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object

262581f

What's New

ab6960f

shoyer reviewed Jul 12, 2019

View reviewed changes

Simplify copy()

678e82b

shoyer reviewed Jul 15, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into object_index

3e7825c

Cosmetic

ae212a0

shoyer approved these changes Aug 2, 2019

View reviewed changes

Guido Imperiale added 2 commits August 2, 2019 14:11

Merge remote-tracking branch 'upstream/master' into object_index

16e229d

Code review

da45c3b

max-sixty merged commit dba85bd into pydata:master Aug 2, 2019

crusaderky deleted the object_index branch August 2, 2019 14:37

astropenguin mentioned this pull request Aug 7, 2019

TypeError: ufunc 'isnan' not supported for the input types, astropenguin/ndradex#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object #3095

Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object #3095

crusaderky commented Jul 11, 2019 •

edited

Loading

codecov bot commented Jul 11, 2019 •

edited

Loading

shoyer Jul 12, 2019

crusaderky Jul 12, 2019 •

edited

Loading

shoyer Jul 13, 2019

crusaderky Jul 13, 2019

shoyer Jul 15, 2019

crusaderky Jul 15, 2019 •

edited

Loading

shoyer Aug 2, 2019

crusaderky commented Jul 19, 2019

crusaderky commented Jul 24, 2019

crusaderky commented Jul 31, 2019

shoyer left a comment

shoyer Aug 2, 2019

crusaderky commented Aug 2, 2019

max-sixty commented Aug 2, 2019

Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object #3095

Fix regression: IndexVariable.copy(deep=True) casts dtype=U to object #3095

Conversation

crusaderky commented Jul 11, 2019 • edited Loading

codecov bot commented Jul 11, 2019 • edited Loading

Codecov Report

shoyer Jul 12, 2019

Choose a reason for hiding this comment

crusaderky Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

shoyer Jul 13, 2019

Choose a reason for hiding this comment

crusaderky Jul 13, 2019

Choose a reason for hiding this comment

shoyer Jul 15, 2019

Choose a reason for hiding this comment

crusaderky Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

shoyer Aug 2, 2019

Choose a reason for hiding this comment

crusaderky commented Jul 19, 2019

crusaderky commented Jul 24, 2019

crusaderky commented Jul 31, 2019

shoyer left a comment

Choose a reason for hiding this comment

shoyer Aug 2, 2019

Choose a reason for hiding this comment

crusaderky commented Aug 2, 2019

max-sixty commented Aug 2, 2019

crusaderky commented Jul 11, 2019 •

edited

Loading

codecov bot commented Jul 11, 2019 •

edited

Loading

crusaderky Jul 12, 2019 •

edited

Loading

crusaderky Jul 15, 2019 •

edited

Loading