Four small issues across the run-orchestration modules. (b) is a real (cosmetic) bug; the rest are hygiene.
(b) run.py writes the job script before validating options — real bug. In toolchain/mfc/run/run.py:
# run.py:181-183
__generate_job_script(targets, case)
__validate_job_options()
__generate_input_files(targets, case)
__validate_job_options() (run.py:20-34) raises MFCException on e.g. --no-mpi with >1 rank, nodes <= 0, or a malformed --email. Because it runs after __generate_job_script has already written the .sh to disk, an invalid invocation aborts leaving a stale job script. Fix: move __validate_job_options() above __generate_job_script(...). (Pure reorder; validation has no side effects.)
(a) ninja/make progress parse duplicated 4× in build.py. _run_build_with_progress parses the ninja regex and the make regex in its non-streaming branch (build.py:73-92) and again, nearly verbatim, in its streaming branch (184-210). → extract _parse_progress(line) -> (completed, total, percent, action) | None used by both branches.
(c) make_options and make_slug share an identical loop body — toolchain/mfc/state.py:42-52 and 54-64. Both build the same --[no-]<cli_k> / --{v}-gpu option list; make_slug only additionally sorts by key and "_".joins. → have make_slug call make_options.
(d) common.quit shadows the builtin — toolchain/mfc/common.py:205:
def quit(sig):
os.kill(os.getpid(), sig)→ rename to s_quit/kill_self and update callers.
Why: (b) leaves confusing artifacts after a failed run; (a)/(c) are copy-paste that must be edited in lockstep; (d) is a readability/lint nicety.
Scope: recommend splitting — (b) as its own small bug-fix PR (clearest, most valuable), and (a)+(c)+(d) bundled as a "run-orchestration tidy-up" PR. Behavior preserved in all four.
Filed from a repo-wide code-cleanliness review; verified against master @ 40dde5e.
Code references
Four small issues across the run-orchestration modules. (b) is a real (cosmetic) bug; the rest are hygiene.
(b) run.py writes the job script before validating options — real bug. In
toolchain/mfc/run/run.py:__validate_job_options()(run.py:20-34) raisesMFCExceptionon e.g.--no-mpiwith >1 rank,nodes <= 0, or a malformed--email. Because it runs after__generate_job_scripthas already written the.shto disk, an invalid invocation aborts leaving a stale job script. Fix: move__validate_job_options()above__generate_job_script(...). (Pure reorder; validation has no side effects.)(a) ninja/make progress parse duplicated 4× in build.py.
_run_build_with_progressparses the ninja regex and the make regex in its non-streaming branch (build.py:73-92) and again, nearly verbatim, in its streaming branch (184-210). → extract_parse_progress(line) -> (completed, total, percent, action) | Noneused by both branches.(c)
make_optionsandmake_slugshare an identical loop body —toolchain/mfc/state.py:42-52and54-64. Both build the same--[no-]<cli_k>/--{v}-gpuoption list;make_slugonly additionally sorts by key and"_".joins. → havemake_slugcallmake_options.(d)
common.quitshadows the builtin —toolchain/mfc/common.py:205:→ rename to
s_quit/kill_selfand update callers.Why: (b) leaves confusing artifacts after a failed run; (a)/(c) are copy-paste that must be edited in lockstep; (d) is a readability/lint nicety.
Scope: recommend splitting — (b) as its own small bug-fix PR (clearest, most valuable), and (a)+(c)+(d) bundled as a "run-orchestration tidy-up" PR. Behavior preserved in all four.
Filed from a repo-wide code-cleanliness review; verified against
master@40dde5e.Code references
toolchain/mfc/run/run.py:181-183— generate-before-validate (bug)toolchain/mfc/build.py:73-92— ninja/make parse (1 of 4 copies)toolchain/mfc/state.py:42-64— make_options / make_slug twinstoolchain/mfc/common.py:205— quit shadows builtin