✨ Add Python bindings for mqt-cc#1815
Conversation
denialhaag
added
feature
denialhaag
force-pushed
the
first-mlir-python-bindings
branch
from
e457bf9 to
f3ca462
Compare
denialhaag
changed the title
mqt-qccmqt-cc
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
denialhaag
force-pushed
the
first-mlir-python-bindings
branch
from
da9a9be to
51d88a9
Compare
This reverts commit 2ef218c.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the ChangesMLIR Python Bindings and Build Enablement
Sequence Diagram(s)sequenceDiagram
participant PythonCaller
participant mqt.core.compile_program
participant moduleFromInputProgram
participant mqt.core.load
participant QuantumCompilerPipeline
participant MLIRText
PythonCaller->>mqt.core.compile_program: program + keyword flags
mqt.core.compile_program->>moduleFromInputProgram: resolve input to MLIR module
moduleFromInputProgram->>mqt.core.load: load QuantumCircuit input
mqt.core.compile_program->>QuantumCompilerPipeline: run configured lowering pipeline
QuantumCompilerPipeline->>MLIRText: capture final IR
MLIRText-->>PythonCaller: return compiled module text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)bindings/mlir/register_mlir.cpp┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.16][ERROR]: unable to find a config; path Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyproject.toml (1)
87-99: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep
build.targetsconsistent with theBUILD_MQT_CORE_MLIRoption.
bindings/CMakeLists.txtonly definesmqt-core-mlir-bindingswhenBUILD_MQT_CORE_MLIRis enabled, but this target is now requested unconditionally here. Any source build that flipsBUILD_MQT_CORE_MLIR=OFFwill still ask scikit-build to build a non-existent target and fail before packaging starts. Either make the target list conditional on the same flag or stop enumerating the MLIR bindings target explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 87 - 99, The build target list is out of sync with the BUILD_MQT_CORE_MLIR option, because build.targets currently always includes mqt-core-mlir-bindings even though bindings/CMakeLists.txt only defines it when that flag is enabled. Update the build.targets configuration to match the same condition used for BUILD_MQT_CORE_MLIR, either by making the target list conditional or by removing the explicit mqt-core-mlir-bindings entry so source builds with the flag disabled do not request a missing target..github/workflows/ci.yml (1)
171-190: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSet explicit
permissions:on these reusable-workflow jobs.Right now each caller inherits the repository default
GITHUB_TOKENscope. On private/org repos that can be broader than needed, which is exactly theexcessive-permissionswarning zizmor is surfacing here. Please declare the minimum permissions on each job beforeuses:and only widen them if the reusable workflow actually needs more than read access.Minimal pattern
python-tests: + permissions: + contents: read uses: munich-quantum-toolkit/workflows/.github/workflows/reusable-python-tests.yml@e7f84f39ce2d3b6c5d1d04526b8f94f98e455143Also applies to: 202-214, 216-226, 234-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 171 - 190, Add explicit minimal permissions to the reusable-workflow jobs in the CI workflow instead of relying on inherited repository defaults, since the current `python-tests` job (and the other cited `uses:` jobs) may be granting excessive `GITHUB_TOKEN` scope. Update each reusable-workflow caller before its `uses:` line to declare only the permissions required by the called workflow, widening them only if a specific job needs more than read access.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/mlir/register_mlir.cpp`:
- Around line 238-243: The dispatch in the binding code currently relies on
program.type().__name__, which can misidentify unrelated classes and miss valid
subclasses or proxies. In the register_mlir.cpp path around
moduleFromQuantumComputation handling, import qiskit.circuit.QuantumCircuit
lazily and switch the QuantumCircuit check to nb::isinstance against that real
type instead of comparing the name string.
- Around line 171-202: In moduleFromString, the early fallback to
moduleFromSourceString is swallowing real file paths with unsupported
extensions, making the unsupported-extension branch unreachable. Update the path
handling so existing file inputs are validated before deciding between
source-string parsing and file parsing, and only treat non-path-like input as
source text. Use the moduleFromString extension checks and the
moduleFromJeffFile/moduleFromMlirFile/moduleFromQasmFile dispatch to ensure
unsupported files reach the runtime_error path instead of being parsed as source
strings.
In `@pyproject.toml`:
- Around line 289-294: The build setup currently fetches setup-mlir from
releases/latest, which makes the installer mutable and non-reproducible. Update
the setup logic in pyproject.toml and the matching Read the Docs config to use a
fixed setup-mlir release URL or pinned version, and add checksum or signature
verification if the project supports it. Keep the existing installation flow
intact while replacing the latest download with a stable, explicitly referenced
release.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 171-190: Add explicit minimal permissions to the reusable-workflow
jobs in the CI workflow instead of relying on inherited repository defaults,
since the current `python-tests` job (and the other cited `uses:` jobs) may be
granting excessive `GITHUB_TOKEN` scope. Update each reusable-workflow caller
before its `uses:` line to declare only the permissions required by the called
workflow, widening them only if a specific job needs more than read access.
In `@pyproject.toml`:
- Around line 87-99: The build target list is out of sync with the
BUILD_MQT_CORE_MLIR option, because build.targets currently always includes
mqt-core-mlir-bindings even though bindings/CMakeLists.txt only defines it when
that flag is enabled. Update the build.targets configuration to match the same
condition used for BUILD_MQT_CORE_MLIR, either by making the target list
conditional or by removing the explicit mqt-core-mlir-bindings entry so source
builds with the flag disabled do not request a missing target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28bf8e10-f083-4423-b51b-2511abc34b2c
📒 Files selected for processing (15)
.github/workflows/ci.yml.readthedocs.yamlCMakeLists.txtUPGRADING.mdbindings/CMakeLists.txtbindings/mlir/CMakeLists.txtbindings/mlir/register_mlir.cppcmake/SetupMLIR.cmakedocs/mlir/index.mddocs/mlir/python_compiler.mdnoxfile.pypyproject.tomlpython/mqt/core/mlir.pyitest/circuits/bell.jefftest/python/test_mlir.py
denialhaag
changed the title
mqt-ccmqt-cc
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/mlir/register_mlir.cpp (1)
100-120: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReject failed parses before returning a null module.
moduleFromMlirFile(),moduleFromQasmString(), andmoduleFromQasmFile()can return a nullOwningOpRef;compileProgram()later dereferencesmodule.get()in the pipeline path, so invalid input can crash the Python process instead of raisingRuntimeError.Proposed fix
[[nodiscard]] mlir::OwningOpRef<mlir::ModuleOp> moduleFromMlirFile(mlir::MLIRContext* context, const std::string& path) { @@ llvm::SourceMgr sourceMgr; sourceMgr.AddNewSourceBuffer(std::move(file), llvm::SMLoc()); - return parseSourceFile<mlir::ModuleOp>(sourceMgr, context); + auto module = parseSourceFile<mlir::ModuleOp>(sourceMgr, context); + if (!module) { + throw std::runtime_error(std::string("Failed to parse MLIR file '") + + path + "'."); + } + return module; } @@ moduleFromQasmString(mlir::MLIRContext* context, const std::string& qasmSource) { - return mlir::qc::translateQASM3ToQC(qasmSource, context); + auto module = mlir::qc::translateQASM3ToQC(qasmSource, context); + if (!module) { + throw std::runtime_error("Failed to translate OpenQASM source to MLIR."); + } + return module; } @@ llvm::SourceMgr sourceMgr; sourceMgr.AddNewSourceBuffer(std::move(file), llvm::SMLoc()); - return mlir::qc::translateQASM3ToQC(sourceMgr, context); + auto module = mlir::qc::translateQASM3ToQC(sourceMgr, context); + if (!module) { + throw std::runtime_error(std::string("Failed to translate OpenQASM file '") + + path + "'."); + } + return module; }Also applies to: 126-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/mlir/register_mlir.cpp` around lines 100 - 120, `moduleFromMlirFile()` and the other parse helpers can return a null `mlir::OwningOpRef`, which later lets `compileProgram()` dereference `module.get()` and crash. Add an explicit null check immediately after parsing in `moduleFromMlirString()`, `moduleFromMlirFile()`, `moduleFromQasmString()`, and `moduleFromQasmFile()`, and throw a `std::runtime_error` with a clear parse-failure message instead of returning the null module. Make sure the pipeline path in `compileProgram()` only proceeds with a valid module.
♻️ Duplicate comments (1)
bindings/mlir/register_mlir.cpp (1)
172-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t classify all one-line strings as filesystem paths.
A valid one-line source string such as
OPENQASM 3.0; qubit[1] q;now reachesstd::filesystem::exists()and fails as “file does not exist” instead of compiling as source. This is the same unresolved path-vs-source dispatch concern discussed earlier, but the current newline-only heuristic still misses valid inline inputs.Safer shape
- if (input.find('\n') != std::string::npos) { + if (input.find('\n') != std::string::npos || + input.find("OPENQASM") != std::string::npos || + input.find("module") != std::string::npos) { return moduleFromSourceString(context, input); }For a fuller fix, dispatch known file extensions as files first, then try
moduleFromSourceString()before probing ambiguous unsupported extensions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/mlir/register_mlir.cpp` around lines 172 - 190, The input dispatch in register_mlir.cpp is treating any one-line string as a filesystem path, so valid inline source like OPENQASM text falls through to std::filesystem::exists() and errors out. Update the path-vs-source logic around moduleFromSourceString and the std::filesystem::path check to prefer known file extensions as files first, then try parsing as source before probing ambiguous inputs with exists(). Keep the current newline and empty-input handling, but make sure one-line source strings are not rejected as missing files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/mlir/register_mlir.cpp`:
- Line 181: The use of std::error_code in the MLIR registration code lacks a
direct standard-library include; add the missing <system_error> include
alongside the other headers in register_mlir.cpp so the type is declared
explicitly. Keep the change localized near the existing includes and ensure the
registration code that uses std::error_code continues to compile cleanly.
---
Outside diff comments:
In `@bindings/mlir/register_mlir.cpp`:
- Around line 100-120: `moduleFromMlirFile()` and the other parse helpers can
return a null `mlir::OwningOpRef`, which later lets `compileProgram()`
dereference `module.get()` and crash. Add an explicit null check immediately
after parsing in `moduleFromMlirString()`, `moduleFromMlirFile()`,
`moduleFromQasmString()`, and `moduleFromQasmFile()`, and throw a
`std::runtime_error` with a clear parse-failure message instead of returning the
null module. Make sure the pipeline path in `compileProgram()` only proceeds
with a valid module.
---
Duplicate comments:
In `@bindings/mlir/register_mlir.cpp`:
- Around line 172-190: The input dispatch in register_mlir.cpp is treating any
one-line string as a filesystem path, so valid inline source like OPENQASM text
falls through to std::filesystem::exists() and errors out. Update the
path-vs-source logic around moduleFromSourceString and the std::filesystem::path
check to prefer known file extensions as files first, then try parsing as source
before probing ambiguous inputs with exists(). Keep the current newline and
empty-input handling, but make sure one-line source strings are not rejected as
missing files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ffc1d8e0-9617-4f32-a6e1-b7f776e754bc
📒 Files selected for processing (3)
.readthedocs.yamlbindings/mlir/register_mlir.cpppyproject.toml
2 participants
Description
This PR adds a first version of Python bindings for
mqt-cc. The implementation does not use the official MLIR Python bindings; instead, it relies on basicnanobindbindings. The official MLIR Python bindings are being explored in #1693.Checklist