Append archive rows with LF, not the csv module's CRLF - #38
Merged
Conversation
append_csv_rows built its csv.DictWriter with the default lineterminator, "\r\n", so every row it appended ended in CRLF on every platform, macOS and Linux included. Both archive paths go through it, `ht run --csv` via append_csv and `ht verify-adapter --csv` via _archive_contract, which is how rows 121-188 of models/result.csv came to be stored with CRLF (086694c, 607c199, d2345d5, f007811). The writer now ends rows in "\n". The file is still opened with newline="", so Windows does not translate it back. test_csv_archive_appends_one_row_per_probe appends twice, once writing the header to a new file and once appending to it, and now also checks that the archive's bytes hold no CRLF. It failed before the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes are regression-tested with no unresolved blocking issues.
Pull request overview
This PR standardizes archived CSV output to use LF line endings and adds regression coverage.
Changes:
- Configures archive
DictWriterrows with\nterminators. - Verifies appended archives contain no CRLF sequences.
File summaries
| File | Description |
|---|---|
tests/test_window.py |
Adds line-ending regression coverage. |
src/hydroturing/report.py |
Writes archive records with LF terminators. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
chrimerss
added a commit
that referenced
this pull request
Sep 11, 2026
antecedent_monotonicity opened its window on the step after the wet variant's last antecedent rain, ten quiet days before the storm, so the wet run's recession through those days was counted as extra storm runoff. The window now opens on the storm itself, and the lower bound is taken as a share of the storm's own rain, which keeps min_share at 0.02: sacsma_snow17 falls below it on 2.0% of 200 seeds, against 11.5% with only the window moved. The six archived rows on the probe are re-run and every verdict is unchanged; wflow_sbm still fails on the same two seeds. The branch already carries main through #36, and main has since gained only #38, which touches no file in common. The merged tree passes pytest (345). model/lisflood, model/cwatm and model/summa still archive rows scored on the old window and need them re-run. Rows 217-223 of models/result.csv were appended through the old CRLF writer; #37 renormalizes them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chrimerss
added a commit
that referenced
this pull request
Sep 11, 2026
#35 replaced rows of models/result.csv that this branch had renormalized, so the merge conflicted there. This branch changes that file only in its line endings (git diff --ignore-cr-at-eol from ec03d0c is empty), so the resolution takes main's rows as they are and ends each in LF. Rows 217-223, which #36 and #35 appended through the CRLF writer that #38 has since fixed, are LF now as well. Ignoring CR at end of line, the merged tree differs from main only in .gitattributes, probe-pr.yml and tests/test_line_endings.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chrimerss
added a commit
that referenced
this pull request
Sep 11, 2026
Text is stored with LF whatever the committer's platform or core.autocrlf: `* text=auto eol=lf` sits above `ht.cmd text eol=crlf`, which still checks out as CRLF, and tests/test_line_endings.py fails when a text file is stored in the index with CRLF or mixed endings. probe-pr.yml now also runs on pull requests that change .gitattributes. On main the one such file was models/result.csv. The branch's renormalization of rows 121-188 had already reached main through #36, and #36 and #35 then appended rows 217-223 through the CRLF writer that #38 has since fixed. 8d5c30c merges main into the branch, resolves the conflict #35 caused in that file to main's rows, and renormalizes the seven. Ignoring CR at end of line, this merge changes only .gitattributes, probe-pr.yml and the new test. The branch head passed pytest (347) locally and in CI. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chrimerss
added a commit
that referenced
this pull request
Sep 11, 2026
docker_runner.build() waits for docker build to exit rather than for its output pipes to close. A docker-credential-desktop helper orphaned while it still holds the build's stderr can no longer stall ht run or ht verify-adapter before any container starts, as it stalled wflow_sbm for seven minutes. The log goes to a temporary file, and a build that never exits ends after 1800 s with an error naming the image and the credential helper. Three tests drive build() with a fake docker. The branch was cut from 096dee9. main has since gained #38, #35 and #37, none of which touches src/hydroturing/runner or tests/test_harness.py, and the pull request's checks started after #37 landed and passed. The merged tree passes pytest (350) and ht validate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chrimerss
added a commit
that referenced
this pull request
Sep 11, 2026
Yuanhang Liu's probe, 705fd5f, proposed and accepted in #19: the same weather run with and without a prescribed net irrigation withdrawal, a paired criterion requiring the difference to account for exactly the abstracted volume, reference_abstraction_blind as its negative control, and the four physical baselines taught to honour the `abstr` column. main gained #38, #35, #37 and #42 after the branch was rebased onto 096dee9, so it cannot be fast-forwarded, and two files conflicted. In models/result.csv, #35's re-scored antecedent rows and this probe's rows were both appended at the end; both are kept. In README.md, the three submitted models' standings: the branch's google_flood_forecast and dhbv2 lines are main's text with the new counts and are taken as they are, but its wflow_sbm line predates #35's rewording, so main's line is kept with the count moved to 14 of 20. Two additions on top of the reviewed branch. wflow_sbm is evaluated on the probe at 1.0.4-ht.2 in Docker: FAIL (VIOLATION). Its own budget closes to 0.02% of the rain, but it never reads the withdrawal and accounts for none of the 380 mm; the row is appended and its standing says so. The Spanish and Chinese site rows for the probe still said return flow must appear in the budget, and now say net withdrawal, as the English row does. The merged tree passes ht validate (20 probes, 32 models), pytest (367 passed) and ht gate, whose output differs from main's only by the new probe's block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xinlan-technology
added a commit
to xinlan-technology/HydroTuring
that referenced
this pull request
Sep 11, 2026
…s LF Upstream landed wflow_sbm (Flood-Lab#36), the antecedent storm-window fix (Flood-Lab#35), LF line endings for every text file with a workflow check that enforces them (Flood-Lab#37, Flood-Lab#38) and the docker build fix (Flood-Lab#42). The conflicts were the three places that count probes: the README's models table, the site's models rows in three languages, and the archive. wflow_sbm's rows take the new probe into their totals, 14 of 20, and its energy wording widens from the three heat-flux probes to the four probes that need an energy output, since it reports neither heat fluxes nor a surface temperature. Its INCOMPLETE row on energy/radiation-consistency is appended by ht run like the others. The archive keeps upstream's renormalised rows followed by this branch's, and the index carries no CRLF.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
append_csv_rowsinsrc/hydroturing/report.pynow passeslineterminator="\n"to itscsv.DictWriter. The csv module's default is\r\n, so every row it appended ended in CRLF on every platform, macOS and Linux included. Both archive paths go through it:ht run --csv(cmd_runviaappend_csv) andht verify-adapter --csv(_archive_contract). That is how rows 121–188 ofmodels/result.csvcame to be stored with CRLF (086694c, 607c199, d2345d5, f007811). The file is still opened withnewline="", so on Windows the\nis written as it is.tests/test_window.py:test_csv_archive_appends_one_row_per_probeappends twice, once writing the header to a new file and once appending to it, and now also asserts that the archive's bytes contain no\r\n.Verification
\r\non macOS. After it, the test passes.ht run --model reference_bucket --probe mass/catchment-closure --seed 11 --csv X, thenht verify-adapter --model reference_streamflow_only --probe mass/catchment-closure --csv X. The file has 3 lines and no\r.detailvalue holding an embedded\n, a", a,, a lone\ror an embedded\r\n, appended in two batches through the sameopenandDictWritercall, reads back unchanged withcsv.DictReaderon Python 3.12 and 3.13; each such field is quoted. In practicedetailis one line: errors keep only their first line. The only reader ofmodels/result.csvin the repository,tests/test_docs_in_sync.py, opens it withnewline=""and accepts either ending.pytest -q: 333 passed (Python 3.12,pip install -e '.[dev]').ruff checkon the two files adds nothing:report.pyhas the same two ISC004 findings it has on main, andtest_window.pyis clean.Other writers checked
Nothing else that writes a committed file uses a csv writer or pandas
to_csv.src/hydroturing/protocol.pywrites the forcing with pandasto_csv, whose default isos.linesep(CRLF on Windows), into the model's/iodirectory. It is a runtime input, not committed, so it is left alone.docs/adapting-a-model.md, writeoutput/result.csvwithcsv.DictWriterinside/io: runtime files, left alone. Theto_csvcalls in the tests write totmp_path.models/google_flood_forecast/event_window_seed598896396.csv, is stored LF, and nothing in the repository writes it.scripts/catchment_from_caravan.py(catchments/<id>.json) andsrc/hydroturing/scaffold.py(probe and model files) write committed files withPath.write_text, which translates\nto\r\non Windows. They write LF on macOS and Linux, and under Store text with LF line endings, and fail CI on committed CRLF #37's* text=auto eol=lfGit converts them on commit. Not changed here.Relation to #37
#37 (
normalize-line-endings, still open) adds* text=auto eol=lf, renormalizes the 68 CRLF rows ofmodels/result.csv, and fails CI when a text file is stored with CRLF. This PR fixes the writer that produced those rows. The two are independent, share no file and merge in either order, but they are related. With #37 alone, Git converts rows appended with CRLF when they are committed, but the working copy is left mixed andgit addwarns that CRLF will be replaced by LF. With this PR alone, new rows are LF, but the rows already stored with CRLF stay that way until #37 renormalizes them.🤖 Generated with Claude Code