Bug Ensure cache_readonly is only applied on instance #40329

fjetter · 2021-03-09T16:21:29Z

The property cache was using the attribute _cache which occasionally is used by other classes as a class attribute. Most prominently this is true for all subclasses of PandasExtensionDtype, see

pandas/pandas/core/dtypes/dtypes.py

Line 89 in ce082fb

_cache: Dict[str_type, PandasExtensionDtype] = {}

This would cause the same cash to be used for the property caching as for the extension dtype. The dtype cache is a class attribute whereas the property cache is supposed to be instance only. I fixed this by using a longer name for the property caching attribute, hoping this would lead to less collisions.

Introducing a new attribute leads to failures for Cython extension types which previously were handled with an AttributeError. Firstly, I believe these cases should not silently pass and secondly the previous behaviour was wrong by not executing the function w/out cache but it returned the instance of the CachedProperty class instead. I addressed this but enabling the caching for extension classes requires defining the attribute explicitly. I went ahead and created a mixin class for this but I'm open to other suggestions as well.

closes N/A
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

fjetter · 2021-03-09T16:21:55Z

pandas/tests/libs/test_lib.py

+        assert instanceA.expensive_property == "A"
+        assert instanceA.counter == 1
+
+        assert instanceB.expensive_property == "B"


on master this returns A

jbrockmendel · 2021-03-09T16:51:05Z

doc/source/whatsnew/v1.3.0.rst

@@ -601,6 +601,7 @@ ExtensionArray

 - Bug in :meth:`DataFrame.where` when ``other`` is a :class:`Series` with :class:`ExtensionArray` dtype (:issue:`38729`)
 - Fixed bug where :meth:`Series.idxmax`, :meth:`Series.idxmin` and ``argmax/min`` fail when the underlying data is :class:`ExtensionArray` (:issue:`32749`, :issue:`33719`, :issue:`36566`)
+- Bug where some properties of subclasses of :class:``PandasExtensionDtype`` where improperly cached :issue:`40329`


where -> were

parens around the issue tag

single-ticks around PandasExtensionDtype

jbrockmendel · 2021-03-09T16:52:27Z

Wouldn't it be simpler to just not instantiate _cache on the class?

fjetter · 2021-03-10T08:08:13Z

Wouldn't it be simpler to just not instantiate _cache on the class?

We don't need a cache per instance not per class.

fjetter · 2021-03-10T13:58:51Z

I believe renaming the property cache is not the best idea since this is used all over the place. The actual problem is that these extension type caches

pandas/pandas/core/dtypes/dtypes.py

Line 89 in 7b5957f

_cache: Dict[str_type, PandasExtensionDtype] = {}

are class attributes but share the same name as the commonly used instance attribute and I do not know how best to distinguish them. I would propose to rename the dtypes cache but I'm not sure if this is perceived as a breaking change or not

jbrockmendel · 2021-03-10T16:00:23Z

I would propose to rename the dtypes cache but I'm not sure if this is perceived as a breaking change or not

It would not. Go for it.

pandas/_libs/properties.pxd

jbrockmendel · 2021-03-22T17:20:59Z

Why not go the other direction and change PandasExtensionDtype._cache to e.g. PandasExtensionDtype._instance_cache

fjetter · 2021-04-05T12:37:31Z

Sorry for not responding. I was out of office for a while.

I added a runtime check to the property cache implementation which is supposed to ensure that we're not accessing any class wide state. I'm also ensuring that we're actually using the cache for ExtensionTypes and raise an appropriate exception if we do not. This led me down the serialization implementation of the Offset classes where most implement a custom serialization function although there is one reduce implemented which should serve all and ensures that the classes are properly instantiated. Otherwise we run into this runtime check I implemented.

@jbrockmendel

jreback · 2021-04-05T16:20:56Z

pandas/_libs/tslibs/offsets.pyx

@@ -895,13 +895,6 @@ cdef class Tick(SingleConstructorOffset):

        raise ApplyTypeError(f"Unhandled type: {type(other).__name__}")

-    # --------------------------------------------------------------------


what were these hitting? or were they at all?

I honestly do not know if these were used or not. I ran into the issue that a lot of the BaseOffset children were not properly initialized. I ended up initializing _cache in every class but noticed they should all do fine using the __reduce__ in

pandas/pandas/_libs/tslibs/offsets.pyx

Lines 728 to 737 in ce34c1c

def __reduce__(self):

# This __reduce__ implementation is for all BaseOffset subclasses

# except for RelativeDeltaOffset

# np.busdaycalendar objects do not pickle nicely, but we can reconstruct

# from attributes that do get pickled.

tup = tuple(

getattr(self, attr) if attr != "calendar" else None

for attr in self._attributes

)

return type(self), tup

They are all tested in

pandas/pandas/tests/io/test_pickle.py

Lines 170 to 177 in ce34c1c

def test_pickles(current_pickle_data, legacy_pickle):

if not is_platform_little_endian():

pytest.skip("known failure on non-little endian")

version = os.path.basename(os.path.dirname(legacy_pickle))

with catch_warnings(record=True):

simplefilter("ignore")

compare(current_pickle_data, legacy_pickle, version)

with the data of

pandas/pandas/tests/io/generate_legacy_storage_files.py

Line 135 in ce34c1c

def create_data():

jbrockmendel · 2021-04-05T16:34:43Z

pandas/_libs/properties.pyx

@@ -23,12 +23,29 @@ cdef class CachedProperty:
            return self

        # Get the cache or set a default one if needed
-        cache = getattr(obj, '_cache', None)
+        cache = instance_cache = getattr(obj, '_cache', None)


why is any of this necessary?

I didn't really know how to test this and implemented this runtime check. I think it's not absolutely necessary if one follows the convention properly but I believe it is very easy to make a mistake here.

def test_no_cache_attribute_on_class(): for cls in list_of_all_relevant_classes_somehow(): assert not hasattr(cls, "_cache")

jbrockmendel · 2021-04-05T16:38:01Z

pandas/core/dtypes/dtypes.py

@@ -86,7 +86,7 @@ class PandasExtensionDtype(ExtensionDtype):
    base: Optional[DtypeObj] = None
    isbuiltin = 0
    isnative = 0
-    _cache: Dict[str_type, PandasExtensionDtype] = {}
+    _cache_dtypes: Dict[str_type, PandasExtensionDtype] = {}


the changes in this file and in pandas/tests/dtypes/test_dtypes.py make sense, the rest i think are unnecessary and in the case of cache_readonly harmful

fjetter · 2021-04-05T16:50:07Z

I get that the complexity in properties.pyx is not necessary. I wanted to have some kind of protection that this is not happening again and this runtime check is the only way I could come up with. Code runs either way with the changes in dtype but I could not come up with another way of testing this behaviour. If this is fine without a dedicated test, I can revert the cython code

fjetter · 2021-04-06T11:29:08Z

I reverted the changes in the cython code base. What's left is the renaming of the ExtensionDtype._cache

jreback · 2021-04-08T22:18:11Z

@fjetter looks fine, can you merge master and ping on green

fjetter · 2021-04-12T12:38:06Z

This was closed by #40193

fjetter commented Mar 9, 2021

View reviewed changes

fjetter force-pushed the cache_readonly_instance branch from b3255dd to 1e41bac Compare March 9, 2021 16:22

fjetter added the Bug label Mar 9, 2021

fjetter mentioned this pull request Mar 9, 2021

PERF: Cache hashing of categories #40193

Merged

4 tasks

fjetter changed the title ~~[Bug] Ensure cache_readonly is only applied on instance~~ Bug Ensure cache_readonly is only applied on instance Mar 9, 2021

fjetter force-pushed the cache_readonly_instance branch 3 times, most recently from 1e0e6da to 7ccc9ca Compare March 9, 2021 16:41

jbrockmendel reviewed Mar 9, 2021

View reviewed changes

fjetter force-pushed the cache_readonly_instance branch 2 times, most recently from fb5661f to d0fbc0b Compare March 10, 2021 12:53

jbrockmendel reviewed Mar 22, 2021

View reviewed changes

pandas/_libs/properties.pxd Outdated Show resolved Hide resolved

fjetter force-pushed the cache_readonly_instance branch from d0fbc0b to 628a509 Compare April 5, 2021 10:17

fjetter force-pushed the cache_readonly_instance branch from 628a509 to abe7603 Compare April 5, 2021 13:46

jreback added Performance Memory or execution speed performance Categorical Categorical Data Type labels Apr 5, 2021

jreback reviewed Apr 5, 2021

View reviewed changes

jbrockmendel reviewed Apr 5, 2021

View reviewed changes

fjetter force-pushed the cache_readonly_instance branch from b46e5ee to 3cd66a2 Compare April 6, 2021 11:27

jreback added this to the 1.3 milestone Apr 8, 2021

fjetter added 2 commits April 9, 2021 10:49

fix cache_readonly

6bc7e0c

Add whatsnew

cf30dc7

fjetter force-pushed the cache_readonly_instance branch from 3cd66a2 to cf30dc7 Compare April 9, 2021 08:49

fjetter closed this Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Ensure cache_readonly is only applied on instance #40329

Bug Ensure cache_readonly is only applied on instance #40329

fjetter commented Mar 9, 2021 •

edited

Loading

fjetter Mar 9, 2021 •

edited

Loading

jbrockmendel Mar 9, 2021

jbrockmendel commented Mar 9, 2021

fjetter commented Mar 10, 2021

fjetter commented Mar 10, 2021

jbrockmendel commented Mar 10, 2021

jbrockmendel commented Mar 22, 2021

fjetter commented Apr 5, 2021

jreback Apr 5, 2021

fjetter Apr 5, 2021

jbrockmendel Apr 5, 2021

fjetter Apr 5, 2021

jbrockmendel Apr 5, 2021

jbrockmendel Apr 5, 2021

fjetter commented Apr 5, 2021

fjetter commented Apr 6, 2021

jreback commented Apr 8, 2021

fjetter commented Apr 12, 2021 •

edited

Loading

		@@ -895,13 +895,6 @@ cdef class Tick(SingleConstructorOffset):

		raise ApplyTypeError(f"Unhandled type: {type(other).__name__}")

		# --------------------------------------------------------------------

	def __reduce__(self):
	# This __reduce__ implementation is for all BaseOffset subclasses
	# except for RelativeDeltaOffset
	# np.busdaycalendar objects do not pickle nicely, but we can reconstruct
	# from attributes that do get pickled.
	tup = tuple(
	getattr(self, attr) if attr != "calendar" else None
	for attr in self._attributes
	)
	return type(self), tup

	def test_pickles(current_pickle_data, legacy_pickle):
	if not is_platform_little_endian():
	pytest.skip("known failure on non-little endian")

	version = os.path.basename(os.path.dirname(legacy_pickle))
	with catch_warnings(record=True):
	simplefilter("ignore")
	compare(current_pickle_data, legacy_pickle, version)

Bug Ensure cache_readonly is only applied on instance #40329

Bug Ensure cache_readonly is only applied on instance #40329

Conversation

fjetter commented Mar 9, 2021 • edited Loading

fjetter Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

jbrockmendel Mar 9, 2021

Choose a reason for hiding this comment

jbrockmendel commented Mar 9, 2021

fjetter commented Mar 10, 2021

fjetter commented Mar 10, 2021

jbrockmendel commented Mar 10, 2021

jbrockmendel commented Mar 22, 2021

fjetter commented Apr 5, 2021

jreback Apr 5, 2021

Choose a reason for hiding this comment

fjetter Apr 5, 2021

Choose a reason for hiding this comment

jbrockmendel Apr 5, 2021

Choose a reason for hiding this comment

fjetter Apr 5, 2021

Choose a reason for hiding this comment

jbrockmendel Apr 5, 2021

Choose a reason for hiding this comment

jbrockmendel Apr 5, 2021

Choose a reason for hiding this comment

fjetter commented Apr 5, 2021

fjetter commented Apr 6, 2021

jreback commented Apr 8, 2021

fjetter commented Apr 12, 2021 • edited Loading

fjetter commented Mar 9, 2021 •

edited

Loading

fjetter Mar 9, 2021 •

edited

Loading

fjetter commented Apr 12, 2021 •

edited

Loading