Skip to content

fix(utils): save_csv writes a blank row between every record on Windows - #785

Open
arthi-arumugam-git wants to merge 1 commit into
cohere-ai:mainfrom
arthi-arumugam-git:fix/save-csv-blank-rows-on-windows
Open

fix(utils): save_csv writes a blank row between every record on Windows#785
arthi-arumugam-git wants to merge 1 commit into
cohere-ai:mainfrom
arthi-arumugam-git:fix/save-csv-blank-rows-on-windows

Conversation

@arthi-arumugam-git

@arthi-arumugam-git arthi-arumugam-git commented Jul 26, 2026

Copy link
Copy Markdown

save_dataset(format="csv") opens the file with open(filepath, "w") and hands it to csv.DictWriter. DictWriter terminates every row with \r\n itself, and on Windows a text stream then translates that \n into \r\n as well, so every row ends up terminated with \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 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, and pandas.read_csv gives back rows of NaN unless skip_blank_lines is set.

The csv docs call this out directly: the file has to be opened with newline="". That is the whole change. save_avro already opens in binary so it was never affected, and I left save_jsonl alone 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 open call itself:

self.assertEqual(opened.call_args.kwargs.get("newline"), "")

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 open test both fail on main and pass with the change.

Notes

  • Only src/cohere/utils.py and tests/ are touched, both listed in .fernignore.
  • mypy . clean across 342 files, tests/test_save_dataset.py 3 passed.
  • I originally included a 7.0.8 -> 7.0.9 bump 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 against utils.py at the same time and they cannot both bump to the same version, and core/client_wrapper.py is 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.
  • Adjacent and deliberately not fixed here: save_csv and save_jsonl both open in text mode without encoding, so they use the locale default and will raise UnicodeEncodeError on 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\n row endings because csv.DictWriter already emits \r\n and the text stream translated newlines again. Exports now open the output file with newline="", matching Python’s csv guidance so each row has a single CRLF.

Adds tests/test_save_dataset.py with byte-level expectations, a csv.DictReader round-trip, and a mocked assertion that open(..., 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.

Comment thread src/cohere/core/client_wrapper.py Outdated
@arthi-arumugam-git

Copy link
Copy Markdown
Author

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.
@arthi-arumugam-git
arthi-arumugam-git force-pushed the fix/save-csv-blank-rows-on-windows branch from b3e75a8 to aa3702c Compare July 26, 2026 07:29
@arthi-arumugam-git

Copy link
Copy Markdown
Author

Pushed a cleanup pass on both this and #784.

Dropped the version bump from both. They cannot both go to 7.0.9, and core/client_wrapper.py is Fern generated rather than in .fernignore, so it is not really mine to edit. That also resolves the User-Agent mismatch Bugbot flagged, since the bump is gone. Each PR now touches only src/cohere/utils.py and tests/, so they no longer conflict with each other and can merge in either order.

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 open is called with newline="", which fails on every platform. Coupling a test to the call shape is not my usual habit and I have said so in a comment next to it, so say the word if you would rather it went.

@arthi-arumugam-git

Copy link
Copy Markdown
Author

Bugbot reviewed 5b287b4, which is no longer in this branch. The rebase at aa3702c drops the version bump and the client_wrapper.py change entirely, so the stale User-Agent it flagged is gone.

Reason for dropping it rather than fixing it: two open PRs cannot both claim 7.0.9, and src/cohere/core/client_wrapper.py is Fern generated rather than hand maintained. Versioning is yours to do at release time.

This PR now touches src/cohere/utils.py and tests/test_save_dataset.py only, both covered by .fernignore. It also means this PR and #784 no longer conflict and can merge in either order.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant