Skip to content

Conversation

@marco66colombo
Copy link
Contributor

@marco66colombo marco66colombo commented Apr 2, 2025

Description

This PR introduces synthesis tests into the pytest framework for hls4ml.

The goal of this change is to automate the validation of HLS synthesis reports against predefined baselines to ensure stability and correctness over time, using pytest.

Main Changes

  • Baselines are shipped with this repository under test/pytest/baselines/<backend>/<version>/<report>.json. Each test case specifies its baseline name, matching the artifact names produced by CI so comparisons stay aligned. Baselines must be refreshed within the PR that changes the synthesis expectations; otherwise the tests will fail.

  • test/pytest/conftest.py fixture: introduces the shared synthesis_config fixture that drives every synthesis test. It gathers:

    • whether synthesis should run (RUN_SYNTHESIS, default false if env var is unset);
    • tool versions (VIVADO_VERSION, VITIS_VERSION, QUARTUS_VERSION, ONEAPI_VERSION) with defaults if the environment variables are unset;
    • backend-specific build arguments.
  • Synthesis helper (test/pytest/synthesis_helpers.py):

    • Main entry point: run_synthesis_test.
    • When run_synthesis is disabled the helper returns early; otherwise it builds, saves the report, and compares it to the selected baseline.
    • Quartus remains skipped, while Vivado, Vitis, and oneAPI execute normally.
    • Missing tools, missing baselines, or mismatched reports trigger pytest.fail(...), so synthesis regressions surface immediately instead of being skipped.
    • The helper persists the generated reports as artifacts, which also helps bootstrap new baselines when they do not exist yet.
  • Tolerances: backend-specific tolerances are still provisional and will need further tuning as we collect more data.

  • CI integration (test/pytest/ci-template.yml):

    • Vivado and Vitis run inside Apptainer containers, sourcing the toolchains from CVMFS. This keeps the CI image smaller while letting us pick tool versions job-by-job.
    • oneAPI remains installed in the image for now and will move to the apptainer container flow in a follow-up.
    • CI stores the synthesis reports as artifacts so we can compare performance against the tracked baselines.
  • generate_ci_yaml.py now has a SPLIT_BY_TEST_CASE map. For any test file listed there, the script inspects its test functions, batches them according to the configured chunk size, and emits separate CI jobs (each passing PYTESTFILE="path::test_fn"). This keeps heavy synthesis suites like test_keras_api sharded without touching the test code.

  • oneAPI report parsing has been hardened to tolerate values such as n/a, avoiding the previous invalid literal for int() with base 10: 'n/a' error.

  • Current test coverage:

    • test_keras_api.py is the first pytest using the synthesis flow. Each parametrized test now takes the synthesis_config fixture, formats a baseline filename (e.g. hls4mlprj_keras_api_dense_{backend}_{io_type}.json), and calls run_synthesis_test(...) after the assertions. Some cases (such as test_conv2d) guard the call with backend/strategy filters to avoid unsupported configurations or to avoid syntehsis that take too long for the jobs.
    • To enable synthesis elsewhere, reproduce the same pattern.
    • Vivado, Vitis, and oneAPI synthesis paths are active; Quartus is currently not supported.

Type of change

  • Other: improvment of testing infrastrucutre using pytest

Tests

Synthesis tests are run conditionally and compared against versioned baselines if RUN_SYNTHESIS=true is set in the environment.

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my feature works.

@marco66colombo marco66colombo self-assigned this Apr 2, 2025
"Vivado": {"csim": False, "synth": True, "export": False},
"Vitis": {"csim": False, "synth": True, "export": False},
"Quartus": {"synth": True, "fpgasynth": False},
"oneAPI": {"build_type": "fpga_emu", "run": False},
Copy link
Contributor

@jmitrevs jmitrevs Apr 7, 2025

Choose a reason for hiding this comment

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

fpga_emu is actually just C compilation, so this needs to be changed, though to what I am not sure. The descriptions are given in https://github.com/oneapi-src/oneAPI-samples/tree/release/2025.0/DirectProgramming/C%2B%2BSYCL_FPGA/Tutorials/GettingStarted/fpga_compile#four-compilation-options and they correspond to fpga_emu, report, fpga_sim, and fpga. I would guess report unless you want to run cosim, in which case it's fpga_sim. The first two don't require Quartus, while the last two do.

@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label Apr 7, 2025
@jmitrevs
Copy link
Contributor

jmitrevs commented Apr 7, 2025

Can we test this to actually run SYNTHESIS tests?

@marco66colombo
Copy link
Contributor Author

yes, but if the env var 'RUN_SYNTHESIS' is not set, the default is False.
I can add a line in the ci-template.yml and export RUN_SYNTHESIS=True

@marco66colombo marco66colombo added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Sep 5, 2025
@marco66colombo marco66colombo added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Sep 8, 2025
@marco66colombo marco66colombo added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Sep 9, 2025
@marco66colombo marco66colombo added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Oct 6, 2025
@marco66colombo marco66colombo added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Oct 14, 2025
@jmitrevs jmitrevs marked this pull request as ready for review October 30, 2025 22:05
@JanFSchulte JanFSchulte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Oct 30, 2025
@JanFSchulte
Copy link
Contributor

Thanks, this looks very nice to me! From the test logs, it seems to have triggered the synthesis tests successfully and the code looks good to me, so I think this is basically ready for merge. I just have a few questions for the way forward:

  • What are the plans to extend this to more tests? How much can we realistically do with the resources we have available?
  • As you said, the thresholds are preliminary, how are these going to be tuned?
  • What's preventing us from testing Quartus right now?

@jmitrevs
Copy link
Contributor

My feeling is that Quartus is basically obsolete now, with the code having migrated to oneAPI. I don't know if we need to spend effort towards setting up synthesis tests for it.

@JanFSchulte
Copy link
Contributor

Fair point. The other thing brought up at the meeting was if we can rebalance the batching of tests somehow? Right now some of them take a few minutes while another takes 1:50h.

@marco66colombo
Copy link
Contributor Author

@JanFSchulte thanks for the review and the follow-up questions.

  • Extending coverage: I’d like to bring synthesis to additional pytest files, but the pace depends on the CERN GitLab runner capacity. I’ll check the available concurrency/quotas and follow up with a better answer for this question. If possible, it would be great to hear which tests the community considers highest priority for synthesis so we can focus on those first.

  • Tuning thresholds: the plan would be to sync with the contributors who regularly use the Vivado/Vitis/oneAPI flows so we can calibrate them against real reports. I’m happy to incorporate suggestions as soon as we have them, since I’m not confident about the best values for every metric, so any feedback is very appreciated.

  • Balancing batches: The longest jobs come both from parametrized tests that explode into many slow combinations and from individual cases that are expensive to synthesize. To tackle that, I’m trimming the heaviest branches directly in the tests with small if guards around run_synthesis_test, and on the CI side this PR introduces SPLIT_BY_TEST_CASE so we can shard the slowest files. I had been experimenting with more advanced batching policies, but I intentionally shipped this simpler version first so we can gather experience and iterate based on the feedbacks. I would like to discuss more on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

please test Trigger testing by creating local PR branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants