Skip to content

Fix .silol.series generation#1591

Merged
sbryngelson merged 4 commits into
MFlowCode:masterfrom
wilfonba:IOFix
Jun 25, 2026
Merged

Fix .silol.series generation#1591
sbryngelson merged 4 commits into
MFlowCode:masterfrom
wilfonba:IOFix

Conversation

@wilfonba

Copy link
Copy Markdown
Contributor

This PR moves the generation of the .silo.series file before the failure message in MFC.sh. If post process failed, then the silo series file wouldn't be generated. I'm not sure how this got reordered, because I thought I'd done the correct ordering when I first merged this. Git blame shows that I just did it wrong though.

@wilfonba wilfonba requested a review from sbryngelson as a code owner June 13, 2026 17:15
Copilot AI review requested due to automatic review settings June 13, 2026 17:15

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR fixes the ordering of .silo.series generation so it happens before the post-process failure path in MFC.sh, ensuring the series file is still produced when post-processing fails.

Changes:

  • Move the generate_silo_series.py invocation earlier in the post_process target block.
  • Ensure .silo.series generation occurs before the $code == 22 early-exit error message.

Comment thread toolchain/templates/include/helpers.mako
@github-actions

github-actions Bot commented Jun 13, 2026

Copy link
Copy Markdown

Claude Code Review

Head SHA: 93497a9

Files changed:

  • 1
  • toolchain/templates/include/helpers.mako

Findings

generate_silo_series.py now runs even when post_process fails

The move places the generate_silo_series.py call before the if [ $code -ne 0 ] guard that previously protected it. In the original code, the script only ran after both error checks ($code -eq 22 and $code -ne 0 ... exit 1), meaning it was effectively run-on-success-only. After this change, the silo series script is invoked regardless of whether post_process succeeded or failed.

On a failed post_process run, calling generate_silo_series.py against incomplete or missing output files may itself error or silently produce a corrupt/empty .visit series file. The overall exit behavior is unchanged (MFC still exits 1 on $code -ne 0), but the unnecessary script call runs and its own exit code is discarded.

If the intent is to generate a partial series on failure (e.g., for debugging), this should be explicit. Otherwise the call should remain after the if [ $code -ne 0 ]; then ... exit 1; fi block, as it was before.

@sbryngelson sbryngelson marked this pull request as draft June 15, 2026 03:38
@sbryngelson sbryngelson marked this pull request as ready for review June 15, 2026 23:26
@sbryngelson

Copy link
Copy Markdown
Member

@wilfonba, what do you think of the Claude review above?

@sbryngelson sbryngelson marked this pull request as draft June 20, 2026 06:00

@sbryngelson sbryngelson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reorder looks right — moving the .silo.series generation ahead of the code -eq 22 / failure-exit blocks ensures it actually runs. One thing I checked that's reassuring: code=$? is captured at line 86, before this python call, so even if generate_silo_series.py errors it won't clobber the real post_process exit status used by the checks below. Minor inline note.

t_${target.name}_stop=$(python3 -c 'import time; print(time.time())')

% if target.name == 'post_process':
python3 "${MFC_ROOT_DIR}/toolchain/templates/include/generate_silo_series.py" '${os.path.dirname(input)}'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Non-blocking: this now runs unconditionally (for post_process) even when post_process actually failed — i.e. it may execute generate_silo_series.py over missing or partial silo output. Since code is already captured above, it won't mask the real error, but worth confirming generate_silo_series.py degrades gracefully (no silo files / incomplete set) rather than dumping a traceback. If you'd rather only generate on success, gating this on [ $code -eq 0 ] would do it — though I suspect you want it to run on the spurious-failure case, which is the whole point of the PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

generate_silo_series.py reads the list of files present in the root directory when it is called, so it should never fail if files exist in the root directory. I don't know what it would do if there were no files in the root directory. I'll have to check the actual script when I'm on the right computer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sbryngelson I tried stress testing this by adding a rm -rf silo_hdf5 in the MFC.sh file before the call to generate_silo_series.py and it fails gracefully. There's an

if not collection_files:
	return

that does an early return if no files are present. This should be fine to merge.

@sbryngelson sbryngelson marked this pull request as ready for review June 25, 2026 16:30
@sbryngelson sbryngelson merged commit ada2764 into MFlowCode:master Jun 25, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants