Skip to content

BUG A file-like object as arg to df.to_csv must have the method getvalue #259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 13, 2018
Merged

BUG A file-like object as arg to df.to_csv must have the method getvalue #259

merged 5 commits into from
Jun 13, 2018

Conversation

jacksonllee
Copy link
Contributor

Since pandas v0.23.1 (specifically from pandas-dev/pandas#21300), if a file-like object is passed into df.to_csv, the object must have the method getvalue, or else an AttributeError is raised (which is what we have observed from #258). An io.TextIOWrapper object doesn't have the method getvalue, which caused the latest build failures in #258. This PR switches to io.StringIO instead.

We need a file-like object with the `getvalue` method
(yes for StringIO, no for TextIOWrapper). Otherwise
when passing a file-like obj to pandas v0.23.1 df.to_csv,
the lack of the getvalue method raises an error.
@jacksonllee
Copy link
Contributor Author

AppVeyor has been turned on, but obviously there's no AppVeyor configuration yet (upcoming from #258) and hence the relevant build "failure", which should be ignored for this PR.

@@ -173,7 +173,7 @@ def test_stash_local_data_from_dataframe_csv(mock_file):
assert _model._stash_dataframe_as_csv(df, mock.Mock()) == -11
mock_file.assert_called_once_with(mock.ANY, name='modelpipeline_data.csv',
client=mock.ANY)
assert isinstance(mock_file.call_args[0][0], BytesIO)
assert isinstance(mock_file.call_args[0][0], StringIO)
Copy link
Contributor

@keithing keithing Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this fail in the Python27 tests? That is, won't this be a BytesIO object for Python27?

Copy link
Contributor Author

@jacksonllee jacksonllee Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! Behold --

Python 2.7, six 1.11.0:

In [2]: from six import StringIO, BytesIO

In [3]: s = StringIO()

In [4]: b = BytesIO()

In [5]: isinstance(s, StringIO)
Out[5]: True

In [6]: isinstance(s, BytesIO)
Out[6]: True

In [7]: isinstance(b, StringIO)  # This is why we didn't get the expected failure?
Out[7]: True

In [8]: isinstance(b, BytesIO)
Out[8]: True

Python 3.6, six 1.11.0 (where everything makes sense to me):

In [1]: from six import StringIO, BytesIO

In [2]: s = StringIO()

In [3]: b = BytesIO()

In [4]: isinstance(s, StringIO)
Out[4]: True

In [5]: isinstance(s, BytesIO)
Out[5]: False

In [6]: isinstance(b, StringIO)
Out[6]: False

In [7]: isinstance(b, BytesIO)
Out[7]: True

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Thanks for looking into that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test passes because of https://github.com/benjaminp/six/blob/1.11.0/six.py#L664 . Given that equality, do we need the if six.PY3 conditional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Although I don't have a problem merging as is, either.

Copy link
Contributor

@keithing keithing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except the single question.

Copy link

@stephen-hoover stephen-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for catching this error and fixing it!

Copy link
Contributor

@keithing keithing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jacksonllee jacksonllee merged commit 028eaf5 into civisanalytics:master Jun 13, 2018
@jacksonllee jacksonllee deleted the stringio-has-getvalue branch June 13, 2018 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants