[Build] Check subprocess return codes in synth/HLS launches - #1662
Open
mumallaeng wants to merge 1 commit into
Open
[Build] Check subprocess return codes in synth/HLS launches#1662mumallaeng wants to merge 1 commit into
mumallaeng wants to merge 1 commit into
Conversation
Several build steps launch Vivado, Vitis, or vitis_hls through subprocess.Popen(...).communicate() and never look at the return code. A failed synthesis, linking, or HLS run is silently treated as success, and the build either proceeds with stale or missing artifacts, or fails later with a confusing, unrelated error. Route these launches through launch_process_helper(check=True), which already exists for this purpose and raises CalledProcessError on a non-zero exit. Also pass check=True on the two rtlsim_exec.py call sites that already use the helper but weren't opting into it. No functional change on success; failures now surface at the point where the tool actually failed. Signed-off-by: mumallaeng <mumallaeng@outlook.com>
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
1 participant
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.
Fix #52.
Problem
Several build steps launch Vivado, Vitis, vitis_hls, or g++ through
subprocess.Popen(bash_command, stdout=subprocess.PIPE); process.communicate()and never check the return code. If the external tool fails, FINN doesn't notice: the build either continues with stale or missing artifacts, or fails later with a confusing error that isn't obviously related to the actual failure upstream.launch_process_helperinfinn/util/basic.pyalready exists to solve exactly this (it raisesCalledProcessErrorwhencheck=Trueand the process exits non-zero), but most of these call sites predate it and never got switched over.Fix
Route every remaining unchecked call site through
launch_process_helper(..., check=True):finn/util/basic.py- CppBuilder.build() (g++ compile)finn/util/hls.py- CallHLS.build() (vitis_hls)finn/custom_op/fpgadataflow/hlsbackend.py- exec_precompiled_singlenode_model()finn/custom_op/fpgadataflow/rtl/finn_loop.py- loop IP project buildfinn/transformation/fpgadataflow/create_stitched_ip.py- stitched IP project buildfinn/transformation/fpgadataflow/make_zynq_proj.py- Zynq synthesisfinn/transformation/fpgadataflow/alveo_build.py- .xo packaging, Vitis link, and report-XML generation (3 call sites)finn/core/rtlsim_exec.py- 2 call sites already using launch_process_helper but not passing check=TrueNo behavior change on the success path. On failure, these now raise CalledProcessError at the point the tool actually failed, instead of silently proceeding.
Testing
isort/black/ruff (pinned versions from .pre-commit-config.yaml) pass on all changed files, and python -m py_compile passes. I don't have a licensed Vivado/Vitis toolchain in this environment to exercise these paths against a real tool failure; the change is a mechanical swap onto the same helper already used elsewhere in the codebase, e.g. alveo_build.py's SLASH link path already does the equivalent with
subprocess.run(..., check=True).