Feature/expanded finn examples - #1637
Conversation
- port the finn-examples build tests onto latest dev (vitis_default_platform rename, path f-strings, resnet50 model name/step_make_driver; xfail kws) - add estimate-level per-PR feature harnesses (dwc/fifo/folding) with shared plumbing and seed references under tests/benchmark/ Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nn-examples Brings the PR base up to date so the branch no longer diverges on the independent dev merges; PR now reflects only the expanded-examples changes.
…ows)
Add tests/benchmark/test_e2e_{baseline,fifo_sizing,folding,generalized_dwc,
combined,label_aligner}.py + _e2e_bench.py + e2e_report.py: each flow builds
every supported finn-examples model (bnn-pynq x6, cybersecurity, vgg10,
mobilenet-v1) through codegen/ipgen/FIFO sizing/stitched IP up to and
including step_measure_rtlsim_performance (no P&R), dumps one JSON per
(flow, model) with rtlsim throughput, estimate-level resources, FIFO KiB and
per-step runtimes, and e2e_report.py assembles the markdown/CSV comparison
table against the baseline flow (folding JSON + largefifo_rtlsim + stock DWC).
Feature flows self-skip on trees without their feature; the baseline flow
refuses to run on a tree carrying the generalized DWC so the reference row
always measures stock DWCs. Optimizer flows run at the folding JSON's own
estimated throughput target (matched-throughput comparison) with padded
folding configs enabled.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The builder's stable_throughput divides the batch by (total - latency) cycles, which degenerates to nonsense when the whole batch drains within the pipeline-fill window (tiny models like tfc). fclk/interval_cycles is the true steady-state rate; fall back to the builder keys when absent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d flow In-loop FIFO scoring multiplies sizing cost by folding_effort; keep the matrix tractable and enable via FINN_E2E_FOLDING_FIFO_HEURISTIC=1. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The optimizer meets its target at the estimate level but the comparison is measured in rtlsim; estimate-vs-rtlsim slack plus discrete fold factors left solutions a few percent short of the baseline's measured throughput (tfc-w1a2: 75 vs 71 cycles/frame). Tunable via FINN_E2E_FOLDING_HEADROOM. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Depths beyond 32768 exceed the RTL FIFO IP cap and fail at CreateStitchedIP (hit by the AlignLabels bypass buffer on cybersecurity-mlp, depth 65536); splitting preserves total storage so the metric is unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… ships cybersecurity-mlp (non-AUP boards) has no committed folding JSON -- its reference config folds via target_fps=1e6. Nulling target_fps there made every cybersec flow measure the unfolded model (and take 1.5h to build). Null target_fps only when a folding_config_file actually exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mobilenet ends in non-dataflow TopK/Flatten before its output; InsertAlignLabels attaches at the graph output and sandwiches them inside the dataflow block (contiguity violation). Needs an output-side skip-non-dataflow rule in InsertAlignLabels to support this topology. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
auphelia
left a comment
auphelia
left a comment
There was a problem hiding this comment.
Thanks @lstasytis for putting this together.
Before diving into specifics, my main concern is that this PR is very bloated for what the base branch sets out to do (add finn-examples build tests). I'd like us to find a leaner shape, and I'll follow up with concrete suggestions on that.
A couple of cross-cutting style points that recur throughout:
- Module naming isn't FINN style. The leading-underscore support modules (_feature_bench.py, _e2e_bench.py) aren't a convention used elsewhere in the tree. An underscore usually marks a package-private member, but these are standalone modules loaded via sys.path.insert(...). Moving the shared scaffolding into a conftest.py (or plainly named modules) under tests/benchmark/ would fix the naming and let you drop the repeated sys.path.insert(0, ...) + # noqa: E402 boilerplate at the top of every test file. You could also use util/test.py
- Copyright headers are inconsistent. Please use the two-liner we have in new files (without year and the full text):
Copyright Advanced Micro Devices, Inc.
SPDX-License-Identifier: BSD-3-Clause
I've left inline comments on specific lines, and more suggestions will follow once we've aligned on the overall scope.
| inst = getCustomOp(n) | ||
| try: | ||
| depths = inst.get_nodeattr("outFIFODepths") | ||
| except (AttributeError, Exception): # noqa: B014 - attr may be absent |
There was a problem hiding this comment.
except (AttributeError, Exception) is equivalent to except Exception. Exception already subsumes AttributeError, so the # noqa: B014 is muting something that should be flagged.
But more importantly, this silently skips any node whose FIFO storage exists but can't be read. Suggest catching only the specific expected case:
try:
depths = inst.get_nodeattr("outFIFODepths")
except AttributeError:
continue
and letting anything unexpected surface.
| return d | ||
|
|
||
|
|
||
| def build_root(): |
There was a problem hiding this comment.
Two concerns here:
- Build artifacts default into the repo tree (REPO_ROOT/e2e_build, and REPO_ROOT/e2e_results in results_dir()). FINN's test guideline is to use scratch/make_build_dir() locations, not the working tree. At minimum these dirs need to be in .gitignore so a stray git add can't commit build output. The /tmp-is-full rationale in the docstring is environment-specific and shouldn't drive mainline defaults.
- os.environ.setdefault("FINN_BUILD_DIR", ...) mutates the process environment as an import/collection-time side effect of a helper. Prefer setting this via monkeypatch.setenv inside a fixture so it's scoped and parallel-safe.
| """Cached rtlsim build. Returns wall-clock seconds (0.0 for a cache hit).""" | ||
| import finn.builder.build_dataflow as build | ||
|
|
||
| os.chdir(REPO_ROOT) |
There was a problem hiding this comment.
os.chdir() changes the working directory for the whole process, which is not safe under pytest-xdist and can leak into unrelated tests. Since the builds already use absolute paths (cfg.output_dir, model_file), consider dropping the chdir entirely, or if a specific CWD is genuinely required, scope it with monkeypatch.chdir() in a fixture.
| return by_model | ||
|
|
||
|
|
||
| def _tp(r): |
There was a problem hiding this comment.
This is a near-verbatim duplicate of stable_throughput() in _e2e_bench.py:372 (same fclk/interval_cycles logic, same fallback chain, same "builder's stable_throughput degenerates" rationale in the comment). Two copies of the steady-state-throughput definition will drift. If the metric is ever refined, one side gets missed and the report silently disagrees with the regression assertion. Suggest exporting the single implementation from _e2e_bench (or wherever the shared scaffolding lands) and importing it here.
2 participants
This PR is an extension of the https://github.com/Xilinx/finn/tree/feature/add_finnexamples branch with the primary goal of becoming a testing harness for the folding, DWC insertion and fifo sizing flows in FINN to make it easier to perform end-to-end tests.
The idea is to provide some ground truths in json format that the flows have to match to test that they work. We can offer two separate modes for each test: slow and fast. Fast would only run up to estimate-report step, avoiding all vivado calls and just testing all the finn machinery for these flows. The slow flow would additionally perform synthesis for all the 3 flows and rtlsim for the fifo sizing one so that we can get a complete test.
Will iterate on this in the coming days.