Skip to content

BUG: refuse to run a Monte Carlo over a results file it cannot write - #1161

Open
thc1006 wants to merge 3 commits into
RocketPy-Team:developfrom
thc1006:bug/refuse-non-jsonl-simulation-logs
Open

BUG: refuse to run a Monte Carlo over a results file it cannot write#1161
thc1006 wants to merge 3 commits into
RocketPy-Team:developfrom
thc1006:bug/refuse-non-jsonl-simulation-logs

Conversation

@thc1006

@thc1006 thc1006 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (ruff check / ruff format --check) has passed locally
  • All tests (pytest tests/unit tests/integration) have passed locally

Current behavior

import_outputs() reads .txt, .csv and .json, sets output_file to the path it read, and its own docstring said the imported file could be used "to continue a simulation".

simulate() only writes JSONL, and __setup_files() opens the file with w+ when append=False. Importing a CSV and running again therefore empties it and writes JSON objects into a file still named .csv:

analysis.import_outputs("results.csv")   # output_file is now results.csv
analysis.simulate(number_of_simulations=100)

Measured on develop: results.csv goes from 31 bytes to 0, then fills with JSONL. The extension, the reader that would be picked for it, and the bytes on disk all disagree, and the imported data is gone.

append=True is the same mismatch without the truncation, appending JSONL rows after a CSV header.

New behavior

simulate() checks all three log paths before anything is opened, and refuses anything that is not .txt. The message names which of input_file, output_file or error_file has to change, and says CSV and JSON stay importable for analysis. Uppercase .TXT is accepted, since it is the same file to the filesystem.

The note under import_outputs no longer offers any saved file as something to resume from, since that is what led a .csv into output_file in the first place.

This does not narrow what can be imported. It only stops simulate() writing over a file it cannot read back.

Breaking change

  • No

A run that pointed a log at .csv or .json was destroying or corrupting that file, so nothing was relying on it.

Additional information

Six tests. Four drive the real simulate() through the monte_carlo_calisto fixture over {.csv, .json} x {append=False, append=True} and assert both that it raises and that read_bytes() is unchanged. The others cover the per-path naming and the uppercase suffix.

Removing the call from simulate() turns only the four integration cases red, while the direct check stays green; making the check accept everything turns both red. That is what separates the wiring from the check.

Local run against develop at 1d04bcc: ruff clean, pylint exit 0, pytest tests/unit tests/integration 2219 passed, 51 skipped.

A format-aware log backend would be the larger fix. This is the preflight that stops data being lost in the meantime.

import_outputs() accepts .csv and .json, points output_file at the file, and
offers continuing a simulation from it. simulate() only writes JSONL, and
__setup_files opens with w+ when append is False, so the imported file was
truncated and then filled with records its own extension does not describe.

Checked before any file is opened, and named per path so the message says which
one has to change.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 requested a review from a team as a code owner August 14, 2026 16:32
@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.55%. Comparing base (e0ff281) to head (b89ca0e).
⚠️ Report is 61 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1161      +/-   ##
===========================================
+ Coverage    82.18%   83.55%   +1.37%     
===========================================
  Files          122      130       +8     
  Lines        16355    17107     +752     
===========================================
+ Hits         13441    14294     +853     
+ Misses        2914     2813     -101     

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

The note under import_outputs said any previously saved file could be used to
continue a simulation, which is what led a .csv into output_file in the first
place. Say which format that holds for.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
import_results points input_file, output_file and error_file at one path, and a
run then appends input rows and output rows into it. Compared by inode once the
files exist, so a symlink, a hard link, a/../run.txt and a case-insensitive
filesystem are all the same file rather than three names.

json.dumps kwargs reach the writer, so indent=2 wrote records across several
lines while every reader here takes one line at a time. The run finished and the
completeness check then called the file it had just written damaged. indent of
0 and "" do the same, as does a newline inside separators.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
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