BUG: fix the bad error raised by HDFStore.put() #38919

1MLightyears · 2021-01-03T12:00:19Z

closes BUG: small error in pandas/io/pytables.py #34274
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry
P.S. something was wrong with git diff upstream/master so I directly ran flake8 ./pandas/io/pytables.py as it's the only file changed

I was running into the same problem as #34274 and found where the error is. But (as I've just started using pandas for several days:sweet_smile:) I have few knowledge about pandas, so maybe I didn't make the full use of pandas' components.

As seen in #34274, if a DataFrame contains non-string elements and is about to be written into an HDF5 file by HDFStore.put(), a TypeError: object of type 'int' has no len() error is raised.

But it's not the right "error" expected. HDFStore.put() can't serialize some types of element, so report an exception on a column by column basis is actually needed.

This commit fixes this, now it raises TypeError: Cannot serialize the column [{column_No}] because its data contents are not string but [{non_string_type}] object dtype as expected.

pep8speaks · 2021-01-03T12:00:25Z

Hello @1MLightyears! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-01-05 00:15:24 UTC

1MLightyears · 2021-01-03T12:57:35Z

The interesting thing is that in the failed tests (like Database / Linux_py37_cov (pull_request)), two of the test case were reported to be wrong, which both include the wrong raised error "object of type 'int' has no len()":
in pandas\tests\io\pytables\test_store.py, line 2052~2060:

# test append with invalid input to get good error messages

# list in column
df = tm.makeDataFrame()
df["invalid"] = [["a"]] * len(df)
assert df.dtypes["invalid"] == np.object_
msg = re.escape("object of type 'int' has no len()")
with pytest.raises(TypeError, match=msg):
    store.append("df", df)

line 2222~2226:

with ensure_clean_store(setup_path) as store:
    # this fails because we have a date in the object block......
    msg = "object of type 'int' has no len()"
    with pytest.raises(TypeError, match=msg):
        store.append("df_unimplemented", df)

should I change them too?

jreback · 2021-01-03T17:21:42Z

The interesting thing is that in the failed tests (like Database / Linux_py37_cov (pull_request)), two of the test case were reported to be wrong, which both include the wrong raised error "object of type 'int' has no len()":
in pandas\tests\io\pytables\test_store.py, line 2052~2060:
# test append with invalid input to get good error messages

# list in column
df = tm.makeDataFrame()
df["invalid"] = [["a"]] * len(df)
assert df.dtypes["invalid"] == np.object_
msg = re.escape("object of type 'int' has no len()")
with pytest.raises(TypeError, match=msg):
    store.append("df", df)
line 2222~2226:
with ensure_clean_store(setup_path) as store:
    # this fails because we have a date in the object block......
    msg = "object of type 'int' has no len()"
    with pytest.raises(TypeError, match=msg):
        store.append("df_unimplemented", df)
should I change them too?

yes these were wrong in the first place, your change should fix them.

pls also add a whatsnew note (1.3 bug fixes I/O)

* Remove bad error raised * Add column label information when raising error for debugging * Update test cases and comments

1MLightyears · 2021-01-04T13:56:57Z

Now it's using a new approach to show the error column label, meanwhile keep the interface part as-is.

The test cases is also updated.

And ... as a new comer, actually I don't exactly know what a whatsnew note is 😅 I copied a commit format from git log and write my own in commit 99005b8, if I made that wrong, pls tell me and I'm willing to correct it.

About the two failed test cases, looks like it's involved with HTTP server, and should not be business of the HDF5 I/O part I commit. No idea about it so far.

jreback

whatsnew note goes here: https://github.com/pandas-dev/pandas/blob/master/doc/source/whatsnew/v1.3.0.rst in bug fixes I/O section

jreback · 2021-01-04T14:14:13Z

pandas/io/pytables.py

+    nan_rep,
+    encoding,
+    errors,
+    block_columns: List[str] = [],


remove the default arg, the annotation is enough

the default [] is used to keep the interface consistency.
but since the function is only called once I think it's okay to remove, maybe
will remove it.

yes and never should you use mutable arguments

jreback · 2021-01-04T14:14:36Z

pandas/io/pytables.py

 ):
+    # block_columns(list[str]): the label of columns for debug info use.


can you add a doc-string template e.g. Parameter / Returns (just list the arg names, and document the new one)

got it.
but giving a private function-which is only internally called once-a docstring looks a bit deluxe...
will add docstring.

jreback · 2021-01-04T14:54:53Z

pandas/io/pytables.py

 ):
+    """
+    Ensure all elements in the given block are "string".


ok this is more confusing, let's just remove this doc-string entirely

sorry english is not my first language and I don't totally understand why non-strings aren't okay to be written in HDF5.
it's just literally do this thing, Ensure all elements in the given block are "string".
It is now removed.

if the elements are not actual strings but python objects we cann't serialize to this column type.

jreback · 2021-01-04T15:21:02Z

doc/source/whatsnew/v1.3.0.rst

@@ -271,6 +271,7 @@ I/O
 - Allow custom error values for parse_dates argument of :func:`read_sql`, :func:`read_sql_query` and :func:`read_sql_table` (:issue:`35185`)
 - Bug in :func:`to_hdf` raising ``KeyError`` when trying to apply
  for subclasses of ``DataFrame`` or ``Series`` (:issue:`33748`).
+- Bug in :func:`put` raising a wrong ``TypeError`` when saving a DataFrame with non-string dtype (:issue:`34274`)


this needs to be :meth:`~HDFStore.put`

say this is an improved error message

Oh thank you for that
It's morning in my timezone here and I just wake up 😅

heheh, no prob this PR looks good. just a small change.

doc/source/whatsnew/v1.3.0.rst

jreback · 2021-01-05T02:22:37Z

thanks @1MLightyears

BUG: fix the bad error raised by HDFStore.put()

5722e6d

fix flake8:line too long

ba55c20

jreback added Error Reporting Incorrect or improved errors from pandas IO HDF5 read_hdf, HDFStore labels Jan 3, 2021

1MLightyears added 3 commits January 4, 2021 19:49

now column label can be showed in error message; update test cases

06ee87c

BUG: fix the bad error raised by HDFStore.put() (pandas-dev#38919)

99005b8

* Remove bad error raised * Add column label information when raising error for debugging * Update test cases and comments

fix format for black

38cf514

jreback requested changes Jan 4, 2021

View reviewed changes

add docstring; update to whatsnew

d9f330a

jreback reviewed Jan 4, 2021

View reviewed changes

docstring of _maybe_convert_for_string_atom() removed

6a93c8e

jreback added this to the 1.3 milestone Jan 4, 2021

jreback requested changes Jan 4, 2021

View reviewed changes

jreback reviewed Jan 5, 2021

View reviewed changes

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

jreback added 2 commits January 4, 2021 19:13

Update doc/source/whatsnew/v1.3.0.rst

4778ba9

Merge branch 'master' into master

9a3ce78

jreback approved these changes Jan 5, 2021

View reviewed changes

jreback merged commit df2c7df into pandas-dev:master Jan 5, 2021

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021

BUG: fix the bad error raised by HDFStore.put() (pandas-dev#38919)

3aac68e

		):
		# block_columns(list[str]): the label of columns for debug info use.

Uh oh!

BUG: fix the bad error raised by HDFStore.put() #38919

BUG: fix the bad error raised by HDFStore.put() #38919

Uh oh!

Conversation

1MLightyears commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Jan 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-01-05 00:15:24 UTC

Uh oh!

1MLightyears commented Jan 3, 2021

Uh oh!

jreback commented Jan 3, 2021

Uh oh!

1MLightyears commented Jan 4, 2021

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jreback commented Jan 5, 2021

Uh oh!

Uh oh!

1MLightyears commented Jan 3, 2021 •

edited

Loading

pep8speaks commented Jan 3, 2021 •

edited

Loading