Fix .silol.series generation#1591
Conversation
There was a problem hiding this comment.
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.pyinvocation earlier in thepost_processtarget block. - Ensure
.silo.seriesgeneration occurs before the$code == 22early-exit error message.
Claude Code ReviewHead SHA: 454d7f6 Files changed:
Findings:
In +% if target.name == 'post_process':
+ python3 "${MFC_ROOT_DIR}/toolchain/templates/include/generate_silo_series.py" '${os.path.dirname(input)}'
+% endif
+
if [ $code -eq 22 ]; then
...
fi
if [ $code -ne 0 ]; then
exit 1
fi
-% if target.name == 'post_process':
- python3 "${MFC_ROOT_DIR}/toolchain/templates/include/generate_silo_series.py" '${os.path.dirname(input)}'
-% endifBecause Running |
|
@wilfonba, what do you think of the Claude review above? |
sbryngelson
left a comment
There was a problem hiding this comment.
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)}' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.