BUG: is_dtype_equal(dtype, "string[pyarrow]") raises if pyarrow not installed #44327

shubham11941140 · 2021-11-05T16:56:31Z

closes BUG: is_dtype_equal(dtype, "string[pyarrow]") raises if pyarrow not installed #44276
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Added instance to check string preventing import error

jreback

pls add a whatsnew note

jreback · 2021-11-06T23:37:43Z

pandas/tests/base/test_misc.py

@@ -150,6 +151,8 @@ def test_access_by_position(index):
    assert index[-1] == index[size - 1]

    msg = f"index {size} is out of bounds for axis 0 with size {size}"
+
+    assert not is_dtype_equal(index.dtype, "string[pyarrow]")


can you move to the tests for is_dtype_equal,

jreback · 2021-11-06T23:38:19Z

pandas/core/dtypes/common.py

@@ -601,7 +601,8 @@ def is_dtype_equal(source, target) -> bool:
            #  is_dtype_equal(CDT, "category") and CDT == "category"
            try:
                src = get_dtype(source)
-                if isinstance(src, ExtensionDtype):
+                if isinstance(src, ExtensionDtype) or isinstance(src, np.dtype):


can you add the example to the doc-string

single isinstance call isinstance(src, (ExtensionDtype, np.dtype))

jreback · 2021-11-06T23:39:14Z

do we actually have a test which exercises this? e.g. not on a build where pyarrow is installed?

shubham11941140 · 2021-11-07T05:36:12Z

do we actually have a test which exercises this? e.g. not on a build where pyarrow is installed?

I do not have pyarrow running on my system and got the initial import error. Actually I am able to run this on a system where pyarrow is not installed. It does not matter as pyarrow is not imported, so installation will not matter.

shubham11941140 · 2021-11-07T05:44:18Z

Whatsnew note added

jbrockmendel · 2021-11-07T19:38:13Z

pandas/tests/dtypes/test_dtypes.py

@@ -115,6 +115,10 @@ def test_construction_from_string(self, dtype):
        with pytest.raises(TypeError, match=msg):
            CategoricalDtype.construct_from_string("foo")

+    def test_pyarrow_string_import_error(self, dtype):
+        # GH-44276


can you flesh this out a tiny bit e.g. 'dont raise if pyarrow is not installed'

doc/source/whatsnew/v1.3.5.rst

jreback · 2021-11-14T02:32:54Z

pandas/tests/dtypes/test_dtypes.py

+    def test_pyarrow_string_import_error(self, dtype):
+        # GH-44276
+        assert not is_dtype_equal(dtype, "string[pyarrow]")
+        with pytest.raises(ImportError, match="pyarrow"):


umm we don't do this, rather use the decorator to skip the test

jreback · 2021-11-14T02:33:14Z

doc/source/whatsnew/v1.3.5.rst

@@ -24,7 +24,7 @@ Fixed regressions

 Bug fixes
 ~~~~~~~~~
-
+- Fixed bug in :bug:`is_dtype_equal` raising if pyarrow not installed (:issue:`44327`)


this goes in 1.4

jreback · 2021-11-14T02:33:29Z

pls merge master as well

jorisvandenbossche · 2021-11-16T10:50:42Z

pandas/tests/dtypes/test_dtypes.py

@@ -115,6 +115,12 @@ def test_construction_from_string(self, dtype):
        with pytest.raises(TypeError, match=msg):
            CategoricalDtype.construct_from_string("foo")

+    def test_pyarrow_string_import_error(self, dtype):


Another note: this test is added to TestCategoricalDtype, so categorical specific tests. I think we should move it to a more generic location. For example in tests/dtypes/test_common.py there is a test_dtype_equal that tests the basic functionality of is_dtype_equal. Maybe add the new test there (or in the arrow string dtype specific tests, @jbrockmendel any preference?)

Request you to inform me where do I exactly put the test, in which file and which function

You can put it in tests/dtypes/test_common.py, just after the existing test_dtype_equal test.

jreback · 2021-11-25T17:29:36Z

pandas/tests/dtypes/test_dtypes.py

+    def test_pyarrow_string_import_error(self, dtype):
+        # GH-44276
+        assert not is_dtype_equal(dtype, "string[pyarrow]")
+        pytest.importorskip("pyarrow")


use the decorator on the function itself to skip

jorisvandenbossche · 2021-11-26T07:16:44Z

pandas/tests/dtypes/test_dtypes.py

@@ -115,6 +115,11 @@ def test_construction_from_string(self, dtype):
        with pytest.raises(TypeError, match=msg):
            CategoricalDtype.construct_from_string("foo")

+    @pytest.importorskip("pyarrow")


I don't think this test should be skipped if pyarrow is not installed? Because I understood the issue that checking the dtype equality currently raises if pyarrow is not installed, while it should return False instead.

So both if pyarrow is installed or not, this test should pass and is_dtype_equal should return False.

I think you can simply remove the decorator.

jorisvandenbossche · 2021-11-26T07:17:30Z

pandas/tests/dtypes/test_dtypes.py

@@ -115,6 +115,12 @@ def test_construction_from_string(self, dtype):
        with pytest.raises(TypeError, match=msg):
            CategoricalDtype.construct_from_string("foo")

+    def test_pyarrow_string_import_error(self, dtype):


You can put it in tests/dtypes/test_common.py, just after the existing test_dtype_equal test.

jorisvandenbossche · 2021-11-26T09:18:57Z

pandas/tests/dtypes/test_common.py

@@ -128,6 +128,12 @@ def test_dtype_equal(name1, dtype1, name2, dtype2):
        assert not com.is_dtype_equal(dtype1, dtype2)


+@pytest.mark.parametrize("name,dtype", list(dtypes.items()), ids=lambda x: str(x))


To the above dtypes dict, can you add an entry for "string": pd.StringDtype() ?

This will be a mistake because isinstance(pd.StringDtype(), str) is False. Adding such an entry in taking to the wrong location in the code segment.

isinstance(pd.StringDtype(), str)

Where is this being checked?

My question is to ensure that is_dtype_equal(pd.StringDtype(), "string[pyarrow]") correctly works, since that was the original bug report.

if isinstance(target, str): if not isinstance(source, str): # GH#38516 ensure we get the same behavior from # is_dtype_equal(CDT, "category") and CDT == "category" # GH-44276 ensures behaviour is consistent # is_dtype_equal (dtype, "string[pyarrow]") try: src = get_dtype(source) if isinstance(src, ExtensionDtype): if isinstance(src, (ExtensionDtype, np.dtype)):

In this code if not isinstance(source, str): returns True when source is pd.StringDtype()

This line returns True for all cases in the current test. Also the other dtype values in the parametrization of this test are actual dtype objects, not strings.

Agreed, but on adding this line into the parameters, this if not isinstance(source, str): returns True which is wrong. Shouldn't isinstance(source, str) be True if source is pd.StringDtype()

No, isinstance(source, str) checks whether source is an actual python string object, while pd.StringDtype is a dtype object, which is a different class, and thus this is expected to return False (the fact that StringDtype is a dtype object for an array that stores strings doesn't matter in that case)

I added the testcase and fixed the Import Bug as well

shubham11941140 · 2021-12-07T14:06:33Z

@jorisvandenbossche @jreback any updates for me on this?

jreback · 2021-12-07T14:08:02Z

doc/source/whatsnew/v1.4.0.rst

@@ -509,7 +509,7 @@ Conversion

 Strings
 ^^^^^^^
-
+- Fixed bug in :bug:`is_dtype_equal` raising if pyarrow not installed (:issue:`44327`)


this won't render & is not public facing anyhow. remove the note

I did not understand the part where "is not public facing anyhow.". I will anyhow remove it.

Do I remove this?

I think you can keep it, but reword it a bit to not mention is_dtype_equal explicitly. Something like "Fixed a bug in checking if a dtype is the string[pyarrow] dtype incorrectly raising an error when pyarrow is not installed (:issue:...)"

jreback · 2021-12-07T14:08:37Z

pandas/core/dtypes/common.py

@@ -596,12 +596,17 @@ def is_dtype_equal(source, target) -> bool:
    False
    """
    if isinstance(target, str):
-        if not isinstance(source, str):
+        if get_dtype(source) == "string":


why is this needed?

It is directly False, there is no need for further checking for this.

the reason i ask is this might be a performance issue here (this code is hit a lot so we care about the code path)

I have changed the code to remove this code segment, now there should not be a problem.

jorisvandenbossche · 2021-12-08T07:36:17Z

pandas/core/dtypes/common.py

-                    return src == target
+                if isinstance(src, (ExtensionDtype, np.dtype)):
+
+                    return type(src) == type(target)


You can't change this to only checking the class of the dtype, we want to check the equality of the instances. See the failing tests

Reverted Back to the Original Code, saw the mistake.

jorisvandenbossche · 2021-12-08T07:37:21Z

pandas/core/dtypes/common.py

-                    return src == target
+                if isinstance(src, (ExtensionDtype, np.dtype)):
+
+                    return type(src) == type(target)
            except (TypeError, AttributeError):


I think you can probably catch the ImportError here (so it always returns False if the above raises an ImportError)

jorisvandenbossche · 2021-12-08T08:21:30Z

pandas/core/dtypes/common.py

            try:
                src = get_dtype(source)
-                if isinstance(src, ExtensionDtype):
+                if isinstance(src, (ExtensionDtype, np.dtype)):


is this change still needed?

It is needed because dtypes such as int64 float64 and other dtypes actually are np.dtype, so if I am not adding this check, the test is failing.

It is needed because dtypes such as int64 float64 and other dtypes actually are np.dtype, so if I am not adding this check, the test is failing.

It is not needed if we catch ImportError more generally, I think; I pushed some small commits to verify this.

shubham11941140 · 2021-12-09T14:35:01Z

@jorisvandenbossche @jreback any updates for me on this?

jreback · 2021-12-14T02:09:23Z

@shubham11941140 you need to merge master

shubham11941140 · 2021-12-14T04:49:12Z

Done.

jorisvandenbossche · 2021-12-15T07:52:58Z

Thanks @shubham11941140

shubham11941140 added 3 commits November 5, 2021 09:54

import error resolved

542032c

pre-commit

76ab6fa

Update test_misc.py

c82d836

jreback requested changes Nov 6, 2021

View reviewed changes

jreback added the Dtype Conversions Unexpected or buggy dtype conversions label Nov 6, 2021

Changes done

c35e162

Duplicate test removed

cff657a

shubham11941140 requested a review from jreback November 7, 2021 05:52

jbrockmendel reviewed Nov 7, 2021

View reviewed changes

Changes done

dce9db6

shubham11941140 requested a review from jbrockmendel November 8, 2021 03:44

jbrockmendel reviewed Nov 8, 2021

View reviewed changes

doc/source/whatsnew/v1.3.5.rst Outdated Show resolved Hide resolved

raises modified

5abfa2e

shubham11941140 requested a review from jbrockmendel November 8, 2021 16:54

jreback requested changes Nov 14, 2021

View reviewed changes

jorisvandenbossche reviewed Nov 16, 2021

View reviewed changes

jorisvandenbossche mentioned this pull request Nov 16, 2021

ENH: allow storing ExtensionArrays in Index #43930

Merged

7 tasks

changes done

fb0bd2a

shubham11941140 requested review from jreback and jorisvandenbossche November 23, 2021 16:27

jreback reviewed Nov 25, 2021

View reviewed changes

decorator fix

f7b8888

shubham11941140 requested a review from jreback November 25, 2021 18:38

jorisvandenbossche requested changes Nov 26, 2021

View reviewed changes

jorisvandenbossche changed the title ~~BUG: is_dtype_equal(dtype, "string[pyarrow]") raises if pyarrow not installedimport error resolved~~ BUG: is_dtype_equal(dtype, "string[pyarrow]") raises if pyarrow not installed Nov 26, 2021

jorisvandenbossche reviewed Nov 26, 2021

View reviewed changes

shubham11941140 requested a review from jorisvandenbossche November 26, 2021 09:51

StringDtype added

9566360

jreback requested changes Dec 7, 2021

View reviewed changes

shubham11941140 requested a review from jreback December 7, 2021 14:12

Try-Except Changed

0f35541

jorisvandenbossche reviewed Dec 8, 2021

View reviewed changes

Changes Done

7ea6b15

shubham11941140 requested a review from jorisvandenbossche December 8, 2021 08:18

jorisvandenbossche reviewed Dec 8, 2021

View reviewed changes

shubham11941140 requested a review from jorisvandenbossche December 8, 2021 08:24

Merge branch 'master' into b5

9b0dc6c

jorisvandenbossche added 5 commits December 14, 2021 14:32

remove np.dtype from isinstance check

b0a75fc

remove np.dtype from isinstance check, catch ImportError instead

75a4b26

update whatsnew

f5bedc6

fix rst syntax in whatsnew

9c68f5c

clean-up

c550a00

jorisvandenbossche approved these changes Dec 15, 2021

View reviewed changes

jorisvandenbossche merged commit 85c221a into pandas-dev:master Dec 15, 2021

jorisvandenbossche added this to the 1.4 milestone Dec 15, 2021

shubham11941140 deleted the b5 branch December 15, 2021 08:27

simonjayhawkins added a commit to simonjayhawkins/pandas that referenced this pull request Dec 15, 2021

DOC: follow up pandas-dev#44327, change issue number

1daa107

simonjayhawkins mentioned this pull request Dec 15, 2021

DOC: follow up #44327, change issue number #44910

Merged

lithomas1 pushed a commit that referenced this pull request Dec 16, 2021

DOC: follow up #44327, change issue number (#44910)

39ccb35

		@@ -128,6 +128,12 @@ def test_dtype_equal(name1, dtype1, name2, dtype2):
		assert not com.is_dtype_equal(dtype1, dtype2)


		@pytest.mark.parametrize("name,dtype", list(dtypes.items()), ids=lambda x: str(x))

BUG: is_dtype_equal(dtype, "string[pyarrow]") raises if pyarrow not installed #44327

BUG: is_dtype_equal(dtype, "string[pyarrow]") raises if pyarrow not installed #44327

Conversation

shubham11941140 commented Nov 5, 2021 • edited Loading

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 6, 2021

shubham11941140 commented Nov 7, 2021

shubham11941140 commented Nov 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham11941140 commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham11941140 commented Dec 9, 2021

jreback commented Dec 14, 2021

shubham11941140 commented Dec 14, 2021

jorisvandenbossche commented Dec 15, 2021

shubham11941140 commented Nov 5, 2021 •

edited

Loading