Conversation
|
@flowerthrower may I ask what the status is for this PR? 😌 |
|
I managed to fix all version incompatibilities. There is only one minor thing I couldn't solve yet; namely the option to print/verify the IR before any pass execution, seems to have been dropped. It's of course not critical to the working of the Plugin but it would be nice for readers of the test files to know how the program looked before the round-trip got executed. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Catalyst dependency from v0.13.0 to v0.14.0, including updates to the LLVM commit hash and related code adaptations to maintain compatibility with the new version.
Changes:
- Updated Catalyst from v0.13.0 to v0.14.0 across all dependency specifications
- Updated LLVM commit hash from
f8cb7987c64dcffb72414a40560055cb717dbf74to113f01aa82d055410f22a9d03b3468fa68600589 - Added support for
memref::CastOpconversion to handle new MLIR patterns from Catalyst v0.14.0 - Updated tests to accommodate changes in Catalyst's MLIR file naming and internal value naming conventions
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updated dependency lock file with new versions of Catalyst (0.14.0), PennyLane (0.44.0), JAX (0.7.1), and introduced new dependencies (xdsl, xdsl-jax, immutabledict, ordered-set) |
| pyproject.toml | Updated Catalyst version constraints and LLVM commit hash in build configuration |
| cmake/ExternalDependencies.cmake | Updated Catalyst version constant |
| CMakeLists.txt | Added LLVM package discovery and include directories |
| README.md | Updated documentation with new Catalyst version and LLVM commit references |
| .readthedocs.yaml | Updated MLIR setup with new LLVM commit hash |
| .github/workflows/*.yml | Updated CI workflows with new LLVM commit hash and Catalyst version |
| lib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpp | Added ConvertMQTOptCast pattern and dynamic legality check for memref::CastOp |
| lib/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.cpp | Enhanced type converter to emit cast operations when types don't match during materialization |
| test/test_plugin.py | Updated test pipelines, MLIR file paths, and CHECK patterns to match new Catalyst behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughUpdated MLIR/LLVM setup hashes and pennylane-catalyst pins across CI/docs; enabled LLVM RTTI/EH and MLIR/LLVM CMake discovery; strengthened memref cast/materialization in two MLIR conversion passes; standardized tests to a three-stage MLIR roundtrip with artifact helpers; removed a reusable MLIR tests workflow. Changes
Sequence Diagram(s)(Skipped — changes are version pins, CI/workflow restructuring, and localized conversion-pattern updates that do not introduce a new multi-component sequential control flow requiring a diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_plugin.py (1)
203-210: Refactor suggestion: centralize intermediate MLIR path discovery.The mlir_dir + three-file pattern is repeated in every test; a helper would reduce duplication and make future filename changes safer.
♻️ Example helper to de-duplicate MLIR path handling
+def _read_roundtrip_mlir_files() -> tuple[str, str, str]: + mlir_dir = Path.cwd() + catalyst_mlir = mlir_dir / "1_AfterInit.mlir" + mlir_to_mqtopt = mlir_dir / "2_AfterToMQTOpt.mlir" + mlir_to_catalyst = mlir_dir / "3_AfterToCatalystQuantum.mlir" + if not catalyst_mlir.exists() or not mlir_to_mqtopt.exists() or not mlir_to_catalyst.exists(): + available_files = list(mlir_dir.glob("*.mlir")) + msg = f"Expected MLIR files not found in {mlir_dir}.\nAvailable files: {[f.name for f in available_files]}" + raise FileNotFoundError(msg) + return ( + catalyst_mlir.read_text(encoding="utf-8"), + mlir_to_mqtopt.read_text(encoding="utf-8"), + mlir_to_catalyst.read_text(encoding="utf-8"), + ) @@ - mlir_dir = Path.cwd() - catalyst_mlir = mlir_dir / "1_AfterInit.mlir" - mlir_to_mqtopt = mlir_dir / "2_AfterToMQTOpt.mlir" - mlir_to_catalyst = mlir_dir / "3_AfterToCatalystQuantum.mlir" - if not catalyst_mlir.exists() or not mlir_to_mqtopt.exists() or not mlir_to_catalyst.exists(): - available_files = list(mlir_dir.glob("*.mlir")) - msg = f"Expected MLIR files not found in {mlir_dir}.\nAvailable files: {[f.name for f in available_files]}" - raise FileNotFoundError(msg) - mlir_before = Path(catalyst_mlir).read_text(encoding="utf-8") - mlir_after_mqtopt = Path(mlir_to_mqtopt).read_text(encoding="utf-8") - mlir_after_roundtrip = Path(mlir_to_catalyst).read_text(encoding="utf-8") + mlir_before, mlir_after_mqtopt, mlir_after_roundtrip = _read_roundtrip_mlir_files()Also applies to: 281-288, 359-366, 430-441, 499-510, 567-578, 637-648, 710-721, 783-794, 855-866, 925-936, 993-1004
🤖 Fix all issues with AI agents
In `@lib/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.cpp`:
- Around line 202-216: In the addTargetMaterialization lambda, remove the
unnecessary braces around the return value that wraps the memref::CastOp (the
return currently written as "return {builder.create<memref::CastOp>(...)}");
change it to return the Value directly from builder.create<memref::CastOp> so
the implicit conversion is used; keep all other logic in the lambda
(inputs.size() check, dyn_cast<MemRefType>(resultType),
isa<MemRefType>(inputs[0].getType())) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmake/ExternalDependencies.cmake (1)
63-68: Early return skips CATALYST_INCLUDE_DIRS setup—build will fail.When
find_package(Catalyst)succeeds, the early return bypasses the Python fallback code that setsCATALYST_INCLUDE_DIRS(line 85). This variable is used by downstream targets (MQTOptToCatalystQuantumandCatalystQuantumToMQTOpt) in theirtarget_include_directories()calls. Either exposeCATALYST_INCLUDE_DIRSfrom the Catalyst CMake module if available, or remove the early return and always execute the Python fallback to ensure the variable is defined.
🤖 Fix all issues with AI agents
In `@lib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpp`:
- Around line 295-314: The memref.cast pattern (ConvertMQTOptCast) and similar
patterns (ConvertMQTOptAlloc, ConvertMQTOptDealloc, ConvertMQTOptStore)
currently use cast<MemRefType> which asserts on unranked memrefs; change these
to use dyn_cast<BaseMemRefType> when retrieving the source and destination types
(e.g., in matchAndRewrite and in the dynamic-legality lambdas) and handle
nullptr (unranked) safely: if either dyn_cast returns null or the element type
is not opt::QubitType, return failure (or mark illegal) as appropriate; update
all corresponding dynamic-legality checks to perform the same
dyn_cast<BaseMemRefType> + element-type test so unranked memref.cast and other
ops are handled without assertions.
In `@test/test_plugin.py`:
- Around line 189-193: Extract the repeated three-stage pipeline list into a
single shared constant or helper function (e.g., DEFAULT_TEST_PIPELINE or
get_default_pipeline()) in test/test_plugin.py and replace the duplicated
literals in tests that currently construct custom_pipeline (the occurrences
around Init/ToMQTOpt/ToCatalystQuantum) with references to that constant/helper;
update any tests that need slight variations to copy or modify the shared
pipeline rather than redefining it inline so all instances (the blocks at ~189,
267, 345, 420, 489, 558, 628, 701, 774, 846, 916, 984) use the centralized
definition and avoid drift.
♻️ Duplicate comments (1)
lib/Conversion/CatalystQuantumToMQTOpt/CatalystQuantumToMQTOpt.cpp (1)
209-212: Simplify the memref::CastOp return.The braced return is unnecessary; returning the
Valuedirectly is clearer.♻️ Minimal cleanup
- return {builder.create<memref::CastOp>(loc, memrefType, inputs[0])}; + return builder.create<memref::CastOp>(loc, memrefType, inputs[0]);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/test_plugin.py`:
- Around line 515-521: Remove the redundant local variable custom_pipeline and
reference the module-level DEFAULT_PIPELINE instead: delete the custom_pipeline
definition and change the qml.qjit decorator call that currently uses
pipelines=custom_pipeline to pipelines=DEFAULT_PIPELINE so the test uses the
centralized DEFAULT_PIPELINE constant (look for the custom_pipeline definition
and the qml.qjit(...) line to update).
♻️ Duplicate comments (2)
test/test_plugin.py (1)
564-570: Remove redundantcustom_pipelinedefinition; useDEFAULT_PIPELINEinstead.Same issue as in
test_rx_gate_roundtrip- this local definition duplicatesDEFAULT_PIPELINE.♻️ Suggested fix
- custom_pipeline = [ - ("Init", ["builtin.module(canonicalize)"]), # Trick to get initial CatalystQuantum MLIR - ("ToMQTOpt", ["builtin.module(catalystquantum-to-mqtopt)"]), - ("ToCatalystQuantum", ["builtin.module(mqtopt-to-catalystquantum)"]), - ] - - `@qml.qjit`(target="mlir", pipelines=custom_pipeline, autograph=True, keep_intermediate=2) + `@qml.qjit`(target="mlir", pipelines=DEFAULT_PIPELINE, autograph=True, keep_intermediate=2)lib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpp (1)
1436-1450: Dynamic legality still misses unranked memref.cast.Using
dyn_cast<MemRefType>treats unranked casts as legal even when they carryopt::QubitType, which can leave illegal types in the converted IR. UseBaseMemRefTypeto cover unranked memrefs consistently withConvertMQTOptCast.🐛 Proposed fix
- target.addDynamicallyLegalOp<memref::CastOp>([](memref::CastOp op) { - auto memrefType = dyn_cast<MemRefType>(op.getType()); - if (!memrefType) { - return true; - } - auto elementType = memrefType.getElementType(); - return !isa<opt::QubitType>(elementType); - }); + target.addDynamicallyLegalOp<memref::CastOp>([](memref::CastOp op) { + auto memrefType = dyn_cast<BaseMemRefType>(op.getType()); + if (!memrefType) { + return true; + } + auto elementType = memrefType.getElementType(); + return !isa<opt::QubitType>(elementType); + });
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)
README.md (1)
175-188: Usekeep_intermediate=3("pass" mode) for guaranteed three MLIR stages.The example uses
keep_intermediate=2("changed" mode in Catalyst 0.14.0), which saves intermediate files only after passes that modify the IR. This does not guarantee the three specific MLIR files shown. To reliably capture all three stages regardless of IR changes, usekeep_intermediate=3("pass" mode) instead. Additionally, verify that the file paths and naming convention match Catalyst 0.14.0's file organization (intermediate files are organized into per-stage subdirectories in this version).
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 113-117: The README currently shows both Unix and Windows
virtualenv activation commands (specifically the lines with "uv venv .venv",
"source .venv/bin/activate", and ".venv\\Scripts\\activate"); since the project
does not support Windows builds, remove the Windows activation hint (the
".venv\\Scripts\\activate" line) or replace it with an explicit note like
"Windows builds unsupported — skip Windows activation" and make the same change
in the other block referenced (the lines around 208-212) so documentation is
consistent about Windows not being supported.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 190-192: Update the output label and/or add a clarifying comment
so users know the MLIR shown is after the initial canonicalize pass: change the
print call that prints "=== Initial Catalyst MLIR ===" (the print that reads
mlir_init via mlir_init.exists() and mlir_init.read_text()) to something like
"=== After Initial Canonicalization (Catalyst MLIR) ===" or add a one-line
comment above it stating that Catalyst v0.14.0 captures the IR after the Init
pipeline's canonicalize pass rather than the raw input.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/Conversion/MQTOptToCatalystQuantum/MQTOptToCatalystQuantum.cpp`:
- Around line 118-123: Update the qubit-type checks used as legality/pattern
guards so they treat both opt::QubitType and catalyst::quantum::QubitType as
qubit element types: in the ConvertMQTOptAlloc, ConvertMQTOptDealloc,
ConvertMQTOptStore, ConvertMQTOptCast pattern guards and the memref::CastOp
dynamic legality, replace single-type checks that only test for opt::QubitType
with a predicate that checks isa<opt::QubitType>(elemType) ||
isa<catalyst::quantum::QubitType>(elemType) (matching the addConversion lambda’s
check), ensuring memref element-type conditions accept already-converted
catalyst::quantum::QubitType and thereby force conversion to
catalyst::quantum::QuregType consistently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 28-32: The RTTI/EH variables (LLVM_ENABLE_RTTI, LLVM_ENABLE_EH)
are currently set before find_package(MLIR) and will be overwritten; move these
set(...) lines to after the find_package(MLIR) call or replace them with a
runtime check that the discovered LLVM/MLIR was built with RTTI/EH enabled
(inspect the variables/targets provided by the found MLIR/LLVM config or use
HandleLLVMOptions to validate and error if requirements aren't met). Ensure you
reference LLVM_ENABLE_RTTI, LLVM_ENABLE_EH, find_package(MLIR), and
HandleLLVMOptions when implementing the change so the intent is enforced against
the found configuration rather than being silently clobbered.
♻️ Duplicate comments (1)
README.md (1)
203-205: Add language specifier to fenced code block.The fenced code block lacks a language identifier, triggering MD040 (fenced-code-language). Add
bashfor consistency with other code blocks.🔧 Proposed fix
-``` +```bash uv run test_example.py</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 86-90: Update the cmake-args value to include the binding build
flag so clang-tidy sees MQT Core Python bindings: append
-DBUILD_MQT_CORE_BINDINGS=ON to the existing cmake-args (the current key named
cmake-args) so it becomes "-DPython3_EXECUTABLE=$VIRTUAL_ENV/bin/python
-DBUILD_MQT_CORE_BINDINGS=ON"; also confirm that the workflow's setup-python
(setup-python: true) actually exports $VIRTUAL_ENV and, if not, explicitly
export/set VIRTUAL_ENV before using it so the Python path in cmake-args points
to the correct virtualenv.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/reusable-python-tests.yml:
- Around line 102-112: The workflow uses a shell-specific variable ($RUNS_ON)
when composing the coverage filename in the "🐍 Test with minimal versions" and
"🐍 Test" steps which breaks on Windows; update those steps to use GitHub
expression expansion instead of shell variables (replace coverage-$RUNS_ON.xml
with coverage-${{ inputs.runs-on }}.xml) or explicitly set the shell to bash for
the run commands so the RUNS_ON env var expands reliably; ensure you update both
occurrences where --cov-report=xml:coverage-...xml appears.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CMakeLists.txt`:
- Around line 21-26: The hardcoded Unix path using $ENV{VIRTUAL_ENV}/bin/python
causes Windows builds to fail; update the logic that sets Python_EXECUTABLE
(when ENV{VIRTUAL_ENV} is defined) to be platform-aware or use CMake's
FindPython venv support: prefer calling find_package(Python COMPONENTS
Interpreter) / set Python_EXECUTABLE via the found Python or, if manually
constructing the path, choose "$ENV{VIRTUAL_ENV}/Scripts/python.exe" on Windows
and "$ENV{VIRTUAL_ENV}/bin/python" on POSIX, ensuring you still cache the result
into Python_EXECUTABLE.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@noxfile.py`:
- Line 35: The CI will fail if Python 3.14 isn't installed because
PYTHON_ALL_VERSIONS includes "3.14" while error_on_missing_interpreters=True;
either remove "3.14" from the PYTHON_ALL_VERSIONS list or ensure the CI workflow
explicitly installs Python 3.14 (e.g., add setup-python@v5 or equivalent to
reusable-python-tests.yml) so that the interpreter exists for nox; update the
PYTHON_ALL_VERSIONS symbol or the CI workflow accordingly and keep
error_on_missing_interpreters behavior unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 133: The current warnings filter string "ignore:.*'return' in a 'finally'
block.*:SyntaxWarning:" is too broad; restrict it to the originating module or
package by adding the module field in the warnings filter (format
action:message:category:module[:lineno]) so only that package is suppressed—e.g.
change the value to include the module pattern (like "ignore:.*'return' in a
'finally' block.*:SyntaxWarning:your.module.or.package") or a package wildcard
(e.g. "yourpackage.*") instead of leaving the module empty, ensuring the filter
only applies to the identified module rather than globally.
Description
This PR updates the Catalyst dependency to v0.14.0, including the new LLVM commit hash.
Checklist: