fix(utils): save_csv writes a blank row between every record on Windows - #785
Conversation
5b287b4 to
b3e75a8
Compare
|
Bot was right, the User-Agent header was still reading cohere/7.0.8 while X-Fern-SDK-Version and pyproject.toml moved to 7.0.9. Pushed a fix so the two strings match. Same note as on #784: both PRs bump 7.0.8 -> 7.0.9, so the second one to merge needs a rebase on that commit. I can rebase or drop the bump entirely, whichever is easier for you. |
save_dataset(format="csv") opens the file with open(filepath, "w") and
hands it to csv.DictWriter. csv.DictWriter terminates every row with
\r\n, and on Windows a text stream then turns the \n into \r\n as well,
so each row ends \r\r\n:
>>> save_csv(dataset, "out.csv")
>>> open("out.csv", "rb").read()
b'text,label\r\r\nhello,a\r\r\ngoodbye,b\r\r\n'
>>> open("out.csv").readlines()
['text,label\n', '\n', 'hello,a\n', '\n', 'goodbye,b\n', '\n']
Python's own csv reader survives this, but anything that reads the file
as text sees an empty row after every record. Excel shows the blank
rows, and pandas.read_csv gives back rows of NaN unless you pass
skip_blank_lines.
The csv docs say the file has to be opened with newline="" for exactly
this reason, so pass it. save_avro already opens in binary and is fine.
On tests: the two that assert file contents only fail on a platform
whose text streams translate newlines, so on Linux CI they would pass
with or without the fix. The third asserts the open call itself, which
fails everywhere without the change, so CI actually guards this rather
than only appearing to.
b3e75a8 to
aa3702c
Compare
|
Pushed a cleanup pass on both this and #784. Dropped the version bump from both. They cannot both go to 7.0.9, and Also added a third test. The two that inspect the written file only fail on Windows, so on Linux CI they would have passed with or without the fix and guarded nothing. The new one asserts that |
|
Bugbot reviewed 5b287b4, which is no longer in this branch. The rebase at aa3702c drops the version bump and the Reason for dropping it rather than fixing it: two open PRs cannot both claim 7.0.9, and This PR now touches |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit aa3702c. Configure here.
save_dataset(format="csv")opens the file withopen(filepath, "w")and hands it tocsv.DictWriter.DictWriterterminates every row with\r\nitself, and on Windows a text stream then translates that\ninto\r\nas well, so every row ends up terminated with\r\r\n.Python's own
csvreader copes with it, so a round trip back through the SDK looks fine and the damage only shows up downstream. Excel renders a blank row after every record, andpandas.read_csvgives back rows ofNaNunlessskip_blank_linesis set.The csv docs call this out directly: the file has to be opened with
newline="". That is the whole change.save_avroalready opens in binary so it was never affected, and I leftsave_jsonlalone since it writes its own separator and is a different question.On the tests, because this one is awkward to guard
The bug only reproduces on a platform whose text streams translate newlines. On Linux the unfixed code writes correct bytes, so any test that inspects the file passes with or without the fix and your CI would be guarding nothing.
So there are three. Two inspect the file, assert exact bytes rather than counting blank rows since that states the real contract, and fail on Windows without the change. The third asserts the
opencall itself:That one fails on every platform, so Linux CI actually catches it if the argument is ever dropped again. Testing the call rather than the output is not something I would normally reach for, and the test carries a comment saying why. Happy to drop it if you would rather not couple a test to the call shape.
Verified in both directions locally: the byte tests and the
opentest both fail onmainand pass with the change.Notes
src/cohere/utils.pyandtests/are touched, both listed in.fernignore.mypy .clean across 342 files,tests/test_save_dataset.py3 passed.7.0.8 -> 7.0.9bump following fix(utils): embed(texts=[]) crashes with IndexError on empty input #778 and feat(client): forward max_retries through Client/ClientV2 constructors #779 and have dropped it. I have a second fix open againstutils.pyat the same time and they cannot both bump to the same version, andcore/client_wrapper.pyis Fern generated rather than hand maintained, so the version looks like yours to set at release. Say the word if you would rather the bump were in here and I will add it back.save_csvandsave_jsonlboth open in text mode withoutencoding, so they use the locale default and will raiseUnicodeEncodeErroron a Windows machine for any dataset containing non cp1252 text. Happy to send that separately if it is worth having.Note
Low Risk
Small, localized change to CSV file writing plus tests; no auth, security, or API contract changes.
Overview
Fixes CSV dataset export on Windows where
save_csv/save_dataset(format="csv")could write\r\r\nrow endings becausecsv.DictWriteralready emits\r\nand the text stream translated newlines again. Exports now open the output file withnewline="", matching Python’scsvguidance so each row has a single CRLF.Adds
tests/test_save_dataset.pywith byte-level expectations, acsv.DictReaderround-trip, and a mocked assertion thatopen(..., newline="")is used so Linux CI still catches regressions if the argument is removed.Reviewed by Cursor Bugbot for commit aa3702c. Bugbot is set up for automated code reviews on this repo. Configure here.