PennyLaneAI / catalyst

A JIT compiler for hybrid quantum programs in PennyLane
https://docs.pennylane.ai/projects/catalyst
Apache License 2.0
142 stars 36 forks source link

Apply clang-tidy to lightning.qubit runtime #1261

Closed paul0403 closed 3 weeks ago

paul0403 commented 3 weeks ago

Context: Catalyst should start conforming to clang-tidy. As a first step, we apply it to the files in the lightning runtime.

Note that this came about both as something we should do anyways, and as a concrete blocker to downstream lightning's catalyst interface to lightning repo (where clang tidy warnings as errors is enabled). We would like to fix all errors reported here.

Description of the Change:

  1. Update .clang-tidy and clang-tidy version in requirements.txt to match those in the Lightning repo
  2. Apply clang-tidy to lightning.qubit runtime. The target files are those in runtime/lib/backend/lightning/lightning_dynamic, alongside some common files in the backend as well.

Benefits: Better adherence to clang rules.

[sc-77220]

github-actions[bot] commented 3 weeks ago

Hello. You may have forgotten to update the changelog! Please edit doc/releases/changelog-dev.md on your branch with:

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 97.95%. Comparing base (0e1c24d) to head (17cca7a). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
runtime/include/QuantumDevice.hpp 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1261 +/- ## ========================================== + Coverage 96.72% 97.95% +1.23% ========================================== Files 56 77 +21 Lines 6800 11319 +4519 Branches 780 981 +201 ========================================== + Hits 6577 11088 +4511 - Misses 173 181 +8 Partials 50 50 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

paul0403 commented 3 weeks ago
  1. Nice work, these files needed to be refactored because Lightning will be fetching the main branch at the moment. However, these very same files will be deleted once the LQ plugin is removed from Catalyst, so this effort will be lost.

I envision the following workflow:

  1. This PR, fixing all errors in lightning's warnings-as-errors, gets merged to catalyst main. At this point, catalyst main has both the common files (like the common/Utils.hpp) and the lightning qubit files, all passing clang tidy.
  2. Lightning PR https://github.com/PennyLaneAI/pennylane-lightning/pull/960 fetches catalyst main, and their CI's clang tidy no longer sees errors on the common files. For the lightning files, at this point I guess you would need to manually "cherry-pick" the changes here into your lightning PR (since they are in different repositories)?
  3. Lightning merges their PR. From lightning's perspective, everything is done. They see common files by fetching catalyst's, and see lightning files in their own repo.
  4. Catalyst merges PR and removes lightning files.

Thoughts?

rauletorresc commented 3 weeks ago
  1. Nice work, these files needed to be refactored because Lightning will be fetching the main branch at the moment. However, these very same files will be deleted once the LQ plugin is removed from Catalyst, so this effort will be lost.

I envision the following workflow:

  1. This PR, fixing all errors in lightning's warnings-as-errors, gets merged to catalyst main. At this point, catalyst main has both the common files (like the common/Utils.hpp) and the lightning qubit files, all passing clang tidy.
  2. Lightning PR Downstream LightningSimulator C++ API to the pennylane-lightning repository pennylane-lightning#960 fetches catalyst main, and their CI's clang tidy no longer sees errors on the common files. For the lightning files, at this point I guess you would need to manually "cherry-pick" the changes here into your lightning PR (since they are in different repositories)?
  3. Lightning merges their PR. From lightning's perspective, everything is done. They see common files by fetching catalyst's, and see lightning files in their own repo.
  4. Catalyst merges PR and removes lightning files.

Thoughts?

I was wrong, the effort is not lost, cause I will update the LQ plugin in Lightning with your changes. Thanks for clarifying that.

The workflow you envision seems correct to me, yes.

paul0403 commented 3 weeks ago

@rauletorresc So I think this is ready to go. In the past couple of commits, I did the following:

  1. https://github.com/PennyLaneAI/catalyst/pull/1261/commits/b38c9a8e120e7471b634280f3c2d64c0f5fdd87c update runtime/.clang-tidy to the same configs as the .clang-tidy in lightning repository
  2. https://github.com/PennyLaneAI/catalyst/pull/1261/commits/722621eb92d1bda54db64d1490a096753510240d enable clang-tidy warnings as errors in catalyst runtime builds
  3. https://github.com/PennyLaneAI/catalyst/pull/1261/commits/1e82397e88cc2970ea48bee9dd335897342461ba Skip openqasm backend builds in CI, since we are only tidying lightning runtime here, so that we don't block ourselves with the warnings-as-errors from openqasm backend
  4. Apply clang tidy suggestions

This is the status of https://github.com/PennyLaneAI/catalyst/pull/1261/commits/690e3dcfd1d1e0637b2dd550b0ac8e287d2bf965. At this point, CI will build lightning runtime, with clang-tidy enabled, and not build openqasm. Runtime builds (with warnings as errors enabled) and tests all succeed. All failing frontend tests are because openqasm backend is not built.

At this point, all the lightning files should be clang-tidy-ed, according to lightning repo's clang tidy config.

If we are good with this, I will:

  1. Turn off clang-tidy in catalyst CI
  2. Turn back on openqasm builds in catalyst CI since catalyst repo as a whole is not ready for clang-tidy yet.

cc @mlxd