Skip to content

fix: drop dtype coercion in CSV update path - #1370

Merged
benoit-cty merged 3 commits into
masterfrom
fix/csv-update-dtype-coercion
Sep 23, 2026
Merged

benoit-cty merged 3 commits into
masterfrom
fix/csv-update-dtype-coercion

Conversation

@davidberenstein1957

@davidberenstein1957 davidberenstein1957 commented Aug 12, 2026 •

Copy link
Copy Markdown
Collaborator

Description

FileOutput.out() in on_csv_write="update" mode previously assigned new values column by column with df[col].dtype.type(val). The single-existing-row branch now drops the previous row for the run and re-appends the new one via pd.concat, achieving the same dedupe-by-run_id without any dtype coercion. The len(df_run) > 1 warning branch is unchanged, and the "no existing row" case falls out of the same concat.

Related Issue

Fixes #1367

Motivation and Context

df[col].dtype.type(val) coerced each incoming value to whatever dtype pandas inferred when reading the existing CSV back. A column that is empty in every existing row is read back as float64, so this evaluated numpy.float64("") or numpy.float64(None) and raised. This is reachable in normal use: an OfflineEmissionsTracker leaves longitude/latitude empty, and gpu_count/gpu_model are empty on CPU-only machines, so any run writing twice (a flush() then stop()) crashed on the second write with ValueError: could not convert string to float: ''.

How Has This Been Tested?

  • New test test_file_output_out_update_with_always_empty_columns fails on master with the ValueError above and passes with this change.
  • uv run pytest tests/output_methods/ -q — 48 passed.
  • uv run pytest tests/test_emissions_tracker.py -q — 31 passed.
  • uv run pre-commit run --files codecarbon/output_methods/file.py tests/output_methods/test_file.py — all hooks pass.

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

AI Usage Disclosure

  • 🟥 AI-vibecoded: You cannot explain the logic. Car analogy : the car drive by itself, you are outside it and just tell it where to go.
  • 🟠 AI-generated: Car analogy : the car drive by itself, you are inside and give instructions.
  • ⭐ AI-assisted. Car analogy : you drive the car, AI help you find your way.
  • ♻️ No AI used. Car analogy : you drive the car.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the docs/how-to/contributing.md document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Note on row ordering

The updated row now moves to the end of the file instead of staying in place. Rows are timestamped, so this only affects files where runs were interleaved. Also note: #1366 (SLURM) touches this same block incidentally; whichever lands second will need a rebase.

@codecov

codecov Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.69%. Comparing base (e5e46ab) to head (6f035dc).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1370      +/-   ##
==========================================
- Coverage   91.70%   91.69%   -0.01%     
==========================================
  Files          49       49              
  Lines        5157     5152       -5     
==========================================
- Hits         4729     4724       -5     
  Misses        428      428              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@davidberenstein1957
davidberenstein1957 marked this pull request as ready for review August 12, 2026 17:36
@davidberenstein1957
davidberenstein1957 requested a review from a team as a code owner August 12, 2026 17:36
davidberenstein1957 added a commit that referenced this pull request Aug 12, 2026
Rebased on fix/csv-update-dtype-coercion (#1370), which fixes the CSV dtype
coercion properly, so the local workaround in file.py is dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidberenstein1957 added a commit that referenced this pull request Aug 19, 2026
Rebased on fix/csv-update-dtype-coercion (#1370), which fixes the CSV dtype
coercion properly, so the local workaround in file.py is dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Updating a run row coerced each incoming value to the dtype pandas inferred
for the existing column. Columns that are empty in every row are read back
as float64, so `numpy.float64("")` / `numpy.float64(None)` raised and the
update failed.

Rebuild the row via concat instead, which dedupes by run_id without touching
dtypes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@davidberenstein1957
davidberenstein1957 force-pushed the fix/csv-update-dtype-coercion branch from 2c331f7 to e056063 Compare August 20, 2026 06:13
@benoit-cty

Copy link
Copy Markdown
Contributor

🤖 This review comment was written and posted by Claude Opus 5.5 (AI assistant), at the request of @benoit-cty. Findings were checked by reading the code and running tests locally (merged with current master where relevant), but please double-check before acting on them.

Verdict: ✅ Approve

What it does: instead of casting each value to the column's dtype, it now drops the old row and appends the new one. This fixes the crash when a column is empty in every row.

What I checked:

  • The bug happens on master. The new test fails there with ValueError: could not convert string to float: '' at codecarbon/output_methods/file.py:123. With this PR, all 24 file-output tests pass.
  • Different column sets: not affected. has_valid_headers already backs up and replaces the file when the columns differ, so the update path only ever sees matching columns.
  • pandas versions: I ran concat of an empty frame plus a row where every column is NA on pandas 2.2.3 and 3.0.5, with -W always. No FutureWarning, and the dtypes are sensible.

Before merge:

  • The branch is behind master; please rebase or update it.

Nits (optional):

  • The updated row now moves to the end of the file. That is fine, and it's already noted in the description.
  • The test uses longitude="". Real OfflineEmissionsTracker output would be None/NaN. Both trigger the bug, so this is only cosmetic.

@davidberenstein1957

Copy link
Copy Markdown
Collaborator Author

Made the changes in 6f035dc: merged master, test now uses None for empty coordinates.

@benoit-cty benoit-cty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks !

@benoit-cty
benoit-cty merged commit 77300ac into master Sep 23, 2026
15 checks passed
@benoit-cty
benoit-cty deleted the fix/csv-update-dtype-coercion branch September 23, 2026 08:53
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.

on_csv_write="update" crashes on second write when a column is empty in all existing rows

2 participants