🚨 Replace mypy with ty for static type checking
#794
Conversation
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
Signed-off-by: burgholzer <[email protected]>
burgholzer
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Warning Rate limit exceeded@denialhaag has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces mypy with ty across CI and pre-commit, adds ty config and dev dependency, expands type overloads in benchmark generation to accept str|QuantumCircuit, refactors module discovery/path handling, adjusts several type annotations and in-place circuit compositions, and updates tests and minor CLI/output runtime checks. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 9
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)
151-163: Consider removing the mypy configuration.Since the PR replaces mypy with ty, the
[tool.mypy]configuration block and overrides may no longer be needed. If there's a reason to keep it (e.g., for developers who want to run both tools or for backward compatibility), please clarify. Otherwise, consider removing lines 151-163 to avoid confusion.src/mqt/bench/benchmarks/shor.py (1)
209-235: Update docstring for_controlled_multiple_mod_nto match return type.The method currently has docstring "Implements modular multiplication by a as an instruction." but returns
QuantumCircuit. Change to "Implements modular multiplication by a as a quantum circuit."All internal callers already treat both
_power_mod_nand_controlled_multiple_mod_nasQuantumCircuitobjects (used withcircuit.compose()), and both methods have correct return type annotations. These are private methods with no external references.
🤖 Fix all issues with AI agents
In @pyproject.toml:
- Line 81: Replace the invalid package version pin "ty==0.0.11" with the
existing PyPI release "ty==0.0.10"; locate the dependency string "ty==0.0.11" in
the pyproject.toml dependencies block and update it to "ty==0.0.10" so the
pinned version is valid.
In @src/mqt/bench/benchmark_generation.py:
- Line 375: The ty:ignore on the pm.run(...) call hides a legitimate type
mismatch because compiled_for_sk.remove_final_measurements(inplace=False) is
typed as returning QuantumCircuit | None; update the code by adding a concise
inline comment explaining that remove_final_measurements(inplace=False) is
guaranteed to return a QuantumCircuit in this code path (never None), or
alternatively perform an explicit runtime check (e.g., assert or raise) before
calling pm.run to satisfy types; reference the call site using
compiled_for_sk.remove_final_measurements(inplace=False) and pm.run so reviewers
can locate and verify the guarantee.
In @src/mqt/bench/benchmarks/__init__.py:
- Around line 16-18: Replace instances that coerce importlib.resources
Traversable objects via Path(str(entry)) with direct Traversable methods: use
entry.is_file(), entry.name, and entry.read_bytes()/read_text() as appropriate
instead of converting to Path; update the discovery loop that currently
references Path(str(entry)) and any file checks to rely on entry.is_file() and
entry.name so resource loading works with non-filesystem backends. Also make the
module import base consistent by using __package__ (or a computed package
string) for resource lookups instead of manipulating __name__; apply the same
changes to the other occurrences flagged (the similar blocks later in the file).
In @src/mqt/bench/benchmarks/shor.py:
- Around line 180-185: The positional assignment of angles via
adder.assign_parameters(angles) is fragile; change append_adder to bind
parameters explicitly by creating a mapping from the adder's parameter objects
to the numeric angles (e.g., use angle_params from the enclosing scope or derive
them deterministically with list(adder.parameters) or [p for p in
adder.parameters if p.name.startswith("angles")] and then call
adder.assign_parameters(dict(zip(angle_params, angles)))); update the call in
append_adder (and ensure angle_params is in scope or computed before
assign_parameters) so parameters are bound by name rather than position.
In @src/mqt/bench/cli.py:
- Line 16: The import currently pulls concrete classes StringIO and
TextIOWrapper; change it to import TextIOBase from io and update any stdout
validation that checks isinstance(..., (StringIO, TextIOWrapper)) to use
isinstance(..., TextIOBase) instead so the check covers all text IO
implementations; adjust the import line replacing StringIO, TextIOWrapper with
TextIOBase and update any references in cli.py that rely on those concrete types
(e.g., stdout validation) to use TextIOBase.
- Around line 138-141: The code uses assert to validate sys.stdout before
calling write_circuit, which is unsafe and skipped with python -O; replace the
assert with an explicit runtime check: if not isinstance(sys.stdout,
(TextIOWrapper, StringIO)): raise TypeError("stdout must be a text stream for
QASM output") (or wrap stdout with an appropriate TextIO if you prefer), then
call write_circuit(circuit, sys.stdout, level, fmt, target) and return;
reference the fmt check, sys.stdout, TextIOWrapper/StringIO and write_circuit to
locate the change.
In @src/mqt/bench/output.py:
- Line 151: Replace the runtime assertions that check stream types (e.g.,
"assert isinstance(destination, TextIOBase), 'QASM output requires a *text*
stream.'") with explicit conditional checks that raise a clear exception
(TypeError or ValueError) when the stream is not a text stream; locate the uses
of destination and TextIOBase in the QASM output code paths and change both the
assert at the shown location and the similar check around line 161 to "if not
isinstance(destination, TextIOBase): raise TypeError('QASM output requires a
text stream, got: ' + type(destination).__name__)" so the validation remains
active even when Python is run with -O.
In @tests/test_bench.py:
- Around line 278-286: The test function's parameter annotation for opt_level is
incorrect: change the type from int to allow None (e.g., use "opt_level: int |
None" or "opt_level: Optional[int]") so it matches the parametrized cases that
pass None; leave the existing runtime handling (opt_level if opt_level is not
None else 2) and imports (add typing.Optional if you choose Optional) and keep
references to get_benchmark unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/ci.yml.pre-commit-config.yamlCHANGELOG.mdpyproject.tomlsrc/mqt/bench/benchmark_generation.pysrc/mqt/bench/benchmarks/__init__.pysrc/mqt/bench/benchmarks/dj.pysrc/mqt/bench/benchmarks/shor.pysrc/mqt/bench/cli.pysrc/mqt/bench/output.pysrc/mqt/bench/targets/devices/__init__.pysrc/mqt/bench/targets/gatesets/__init__.pysrc/mqt/bench/targets/gatesets/ionq.pysrc/mqt/bench/targets/gatesets/rigetti.pytests/test_bench.pytests/test_cli.py
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1337
File: pyproject.toml:351-351
Timestamp: 2025-11-28T14:33:15.199Z
Learning: In the munich-quantum-toolkit/core repository, the dev dependency "ty==0.0.1a27" is intentionally pinned to an exact pre-release version. This dependency is used by the `ty-check` pre-commit hook (via `uv run --only-dev ty check`), and the exact pin ensures all developers run the same version, preventing unexpected issues from updates since ty is in alpha and changing rapidly.
📚 Learning: 2025-11-28T14:33:15.199Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1337
File: pyproject.toml:351-351
Timestamp: 2025-11-28T14:33:15.199Z
Learning: In the munich-quantum-toolkit/core repository, the dev dependency "ty==0.0.1a27" is intentionally pinned to an exact pre-release version. This dependency is used by the `ty-check` pre-commit hook (via `uv run --only-dev ty check`), and the exact pin ensures all developers run the same version, preventing unexpected issues from updates since ty is in alpha and changing rapidly.
Applied to files:
pyproject.toml.pre-commit-config.yamlCHANGELOG.md
📚 Learning: 2026-01-09T00:27:20.419Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 39
File: .pre-commit-config.yaml:135-135
Timestamp: 2026-01-09T00:27:20.419Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, `.pre-commit-config.yaml` intentionally uses `https://github.com/adhtruong/mirrors-typos` instead of the official `https://github.com/crate-ci/typos` repository to avoid pre-commit autoupdate tag confusion described in https://github.com/crate-ci/typos/issues/390.
Applied to files:
pyproject.toml.pre-commit-config.yaml
📚 Learning: 2025-11-01T15:57:31.153Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1283
File: src/qir/runtime/QIR.cpp:196-201
Timestamp: 2025-11-01T15:57:31.153Z
Learning: In the QIR runtime (src/qir/runtime/QIR.cpp), the PRX gate (__quantum__qis__prx__body) is an alias for the R gate (Phased X-Rotation) and should call runtime.apply<qc::R>(theta, phi, qubit), not runtime.apply<qc::RX>() which is a single-parameter rotation gate.
Applied to files:
src/mqt/bench/targets/gatesets/rigetti.py
📚 Learning: 2025-10-09T13:20:11.483Z
Learnt from: DRovara
Repo: munich-quantum-toolkit/core PR: 1108
File: mlir/test/Dialect/MQTOpt/Transforms/lift-measurements.mlir:269-288
Timestamp: 2025-10-09T13:20:11.483Z
Learning: In the MQT MLIR dialect, the `rz` gate should not be included in the `DIAGONAL_GATES` set for the `ReplaceBasisStateControlsWithIfPattern` because its operator matrix does not have the required shape | 1 0 | / | 0 x | for the targets-as-controls optimization. It is only included in `LiftMeasurementsAboveGatesPatterns` where the matrix structure requirement differs.
Applied to files:
src/mqt/bench/targets/gatesets/rigetti.py
📚 Learning: 2025-10-14T14:37:38.047Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-10-11T13:21:15.212Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/ddsim PR: 674
File: .github/workflows/cd.yml:56-56
Timestamp: 2025-10-11T13:21:15.212Z
Learning: In the ddsim repository, modifying .github/workflows/cd.yml intentionally triggers CD workflow testing in CI through a change-detection mechanism in ci.yml. The change-detection job outputs a run-cd flag that controls whether build-sdist and build-wheel jobs execute in CI, allowing testing of the CD workflow (including wheel building and testing) before actual releases.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2025-12-02T10:08:36.022Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 1
File: .github/workflows/cd.yml:40-48
Timestamp: 2025-12-02T10:08:36.022Z
Learning: In munich-quantum-toolkit workflows, the reusable-python-packaging-sdist.yml workflow uploads the sdist artifact with the name `cibw-sdist` (or `dev-cibw-sdist` for pull requests), which means it is covered by the `cibw-*` glob pattern when downloading artifacts for deployment.
Applied to files:
.github/workflows/ci.yml
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
src/mqt/bench/benchmarks/__init__.py
📚 Learning: 2025-12-15T01:59:17.023Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: python/mqt/core/ir/operations.pyi:9-16
Timestamp: 2025-12-15T01:59:17.023Z
Learning: In the munich-quantum-toolkit/core repository, stub files (.pyi) are auto-generated by nanobind's stubgen tool and should not be manually modified for style preferences, as changes would be overwritten during regeneration.
Applied to files:
src/mqt/bench/targets/gatesets/ionq.py
🧬 Code graph analysis (3)
src/mqt/bench/targets/devices/__init__.py (1)
src/mqt/bench/targets/devices/_registry.py (3)
device_names(59-61)get_device_by_name(44-56)register_device(21-41)
tests/test_bench.py (4)
src/mqt/bench/benchmarks/__init__.py (1)
create_circuit(104-130)src/mqt/bench/benchmark_generation.py (5)
get_benchmark(477-487)get_benchmark(491-501)get_benchmark(505-515)get_benchmark(518-592)BenchmarkLevel(39-45)src/mqt/bench/output.py (2)
generate_header(57-103)OutputFormat(34-43)src/mqt/bench/benchmarks/_registry.py (1)
register_benchmark(36-57)
src/mqt/bench/benchmark_generation.py (1)
src/mqt/bench/configuration_options.py (1)
ConfigurationOptions(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
🔇 Additional comments (34)
.github/workflows/ci.yml (1)
57-59: LGTM!The CI configuration correctly enables ty and disables mypy in the linter job, aligning with the PR objectives.
pyproject.toml (3)
137-139: Clarify the purpose of the coverage configuration change.The addition of
run.disable_warningswith"no-sysmon"appears unrelated to the mypy→ty migration. Is this addressing warnings that appeared during testing with ty, or is this a separate fix that should be documented?
271-271: LGTM!The comment correctly reflects that ty is now used instead of mypy for the PC140 repo-review check exemption.
276-283: LGTM!The ty configuration is properly structured:
error-on-warning = trueensures strict type checking- Excludes
docs/**consistently with the pre-commit hook configuration.pre-commit-config.yaml (2)
14-14: LGTM!Correctly updated the CI skip list to skip ty-check instead of mypy, ensuring the check runs in GitHub Actions CI where the environment is properly configured.
94-103: LGTM!The local ty-check hook is properly configured:
- Uses
uv run ty checkwhich leverages the ty dependency from pyproject.tomlrequire_serial: trueensures type checking runs sequentially- File type coverage (
python, pyi, jupyter) is comprehensive- Exclude pattern matches the ty configuration in pyproject.toml
Based on learnings, this local hook pattern with exact version pinning ensures all developers run the same ty version.
CHANGELOG.md (2)
12-15: LGTM!The changelog entry is properly formatted and categorized under "Changed", following the project's changelog template with the correct PR reference and author attribution.
96-96: LGTM!The PR reference link is correctly added and properly formatted.
src/mqt/bench/targets/gatesets/rigetti.py (1)
32-32: LGTM! Type ignore comments successfully removed.The removal of
# type: ignore[misc]comments from the three gate class declarations indicates thattysuccessfully resolves the type checking issues that previously required suppression. The class implementations remain unchanged and correctly inherit fromGate.Also applies to: 64-64, 96-96
src/mqt/bench/benchmarks/dj.py (2)
13-13: LGTM! Proper use of TYPE_CHECKING for Gate import.The TYPE_CHECKING block correctly guards the Gate import to avoid runtime overhead while enabling precise type hints.
Also applies to: 20-22
24-24: LGTM! Type refinement correctly reflects Qiskit's types.The updated signatures accurately reflect that
to_gate()returns aGate(line 52) andappend()accepts aGate(line 67). This improves type precision without changing runtime behavior.Also applies to: 57-57
src/mqt/bench/targets/devices/__init__.py (2)
17-17: LGTM! Path import correctly moved to runtime scope.Moving
PathfromTYPE_CHECKINGto runtime scope is necessary since it's now used in the runtime logic at line 28 (Path(str(entry))).
27-28: Unable to proceed: No review comment provided for verification. Please provide the review comment within<review_comment>tags along with the relevant code context or file information.src/mqt/bench/targets/gatesets/__init__.py (3)
17-17: LGTM! Path import correctly moved to runtime scope.Moving
PathfromTYPE_CHECKINGto runtime scope is necessary since it's now used in the runtime logic at line 33.
32-33: Module discovery pattern is correctly implemented with consistent type handling.The gatesets module properly uses
ir.files(__name__)with explicitPath(str(entry))conversion for type safety, matching the identical pattern in the devices module. Module discovery correctly identifies available gateset modules by filename stem.
107-114: The implementation is safe and well-designed. Each call to the publicget_target_for_gateset()returns a deepcopy of the entire Target object, ensuring users receive independent instances with no shared mutable state. While the cached_get_target_for_gateset()does reuse gate instances within a single Target, this is not problematic because each gate is added to the target exactly once and Qiskit'sTarget.add_instruction()does not mutate instructions. The refactoring from lambda functions to direct instantiation is actually an improvement—it's more efficient while maintaining correctness.src/mqt/bench/output.py (2)
23-23: LGTM! More explicit import of__qiskit_version__.Importing directly from
qiskit.versionis more explicit and clear than importing from the top-levelqiskitmodule.
120-120: LGTM! TextIOBase is more precise than TextIO.Changing from
TextIO(a type alias) toTextIOBase(the actual abstract base class) improves type precision and aligns with runtime type checks at lines 151 and 161.Also applies to: 130-130
src/mqt/bench/targets/gatesets/ionq.py (1)
66-66: Removal of# type: ignore[misc]on Gate subclasses looks safe (no behavior change).
If any residual mypy step still exists in CI/pre-commit, this may re-surface prior diagnostics—worth confirming the migration fully removed mypy checks.Also applies to: 99-99, 135-135, 207-207
src/mqt/bench/benchmarks/shor.py (3)
156-158: In-place compose + explicitreturn circuitis an improvement (avoids accidentalNoneflows).
206-207: Returning the circuit afteriqftinplace compose is consistent and clearer.
299-301: Final inverse-QFTcompose(..., inplace=True)+return circuitis correct and avoids misuse ofcompose’s return value.tests/test_cli.py (1)
23-25: This review comment is based on incorrect assumptions about the pytest-console-scripts API.
RunResultis a public class exported frompytest-console-scripts(defined in the module's__init__.py). There is noScriptResulttype in the library's public API—RunResulthas always been the standard return type. The project constraintpytest-console-scripts>=1.4.1guaranteesRunResultis available. The import at line 24 is correct and requires no fallback or alias logic.Likely an incorrect or invalid review comment.
tests/test_bench.py (5)
187-192: LGTM!The conditional handling properly distinguishes between cases where
kindis provided vs.None, avoiding passing an explicitkind=Nonekeyword argument when the benchmark doesn't support that parameter.
545-545: LGTM!Using
BenchmarkLevel.INDEPenum correctly aligns with thegenerate_headerfunction signature.
945-945: LGTM!The assertion is appropriate in test code to document the precondition that
target_objmust not beNoneat this point in the test for the MAPPED level.
1014-1025: LGTM!Using a variable for the benchmark name improves maintainability by avoiding string literal repetition.
1046-1046: LGTM!The parameter type change to
straligns with the benchmark naming convention used throughout the test suite.src/mqt/bench/benchmark_generation.py (6)
189-198: LGTM!The catch-all overload with
str | QuantumCircuitandint | Noneis a standard pattern to satisfy type checkers when the specific overload cannot be statically resolved. Based on learnings, ty is in alpha and requires these union overloads to handle mixed-type call sites.
250-259: LGTM!Consistent catch-all overload pattern for
get_benchmark_indep.
319-330: LGTM!Consistent catch-all overload pattern for
get_benchmark_native_gates.
137-138: LGTM!The additional null check for
target_qc.layoutis a good defensive measure. After transpilation, the resulting circuit's layout could differ from the input, and this guard prevents potentialAttributeErrorif the transpiled circuit has no layout.
419-430: LGTM!Consistent catch-all overload pattern for
get_benchmark_mapped.
504-516: LGTM!Consistent catch-all overload pattern for
get_benchmark.
| if fmt in (OutputFormat.QASM2, OutputFormat.QASM3) and not args.save: | ||
| assert isinstance(sys.stdout, (TextIOWrapper, StringIO)) | ||
| write_circuit(circuit, sys.stdout, level, fmt, target) | ||
| return |
There was a problem hiding this comment.
Replace assert on sys.stdout with a deterministic runtime error (don’t rely on assert).
Today this can crash as AssertionError (and is skipped under python -O). Use an explicit check + TypeError (or wrap) so CLI behavior is stable and user-friendly.
Proposed patch
-from io import StringIO, TextIOWrapper
+from io import TextIOBase
@@
# For QASM outputs, serialize and print
if fmt in (OutputFormat.QASM2, OutputFormat.QASM3) and not args.save:
- assert isinstance(sys.stdout, (TextIOWrapper, StringIO))
+ if not isinstance(sys.stdout, TextIOBase):
+ msg = f"stdout must be a text stream for {fmt.value} output, got {type(sys.stdout)!r}"
+ raise TypeError(msg)
write_circuit(circuit, sys.stdout, level, fmt, target)
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if fmt in (OutputFormat.QASM2, OutputFormat.QASM3) and not args.save: | |
| assert isinstance(sys.stdout, (TextIOWrapper, StringIO)) | |
| write_circuit(circuit, sys.stdout, level, fmt, target) | |
| return | |
| if fmt in (OutputFormat.QASM2, OutputFormat.QASM3) and not args.save: | |
| if not isinstance(sys.stdout, TextIOBase): | |
| msg = f"stdout must be a text stream for {fmt.value} output, got {type(sys.stdout)!r}" | |
| raise TypeError(msg) | |
| write_circuit(circuit, sys.stdout, level, fmt, target) | |
| return |
🤖 Prompt for AI Agents
In @src/mqt/bench/cli.py around lines 138 - 141, The code uses assert to
validate sys.stdout before calling write_circuit, which is unsafe and skipped
with python -O; replace the assert with an explicit runtime check: if not
isinstance(sys.stdout, (TextIOWrapper, StringIO)): raise TypeError("stdout must
be a text stream for QASM output") (or wrap stdout with an appropriate TextIO if
you prefer), then call write_circuit(circuit, sys.stdout, level, fmt, target)
and return; reference the fmt check, sys.stdout, TextIOWrapper/StringIO and
write_circuit to locate the change.
There was a problem hiding this comment.
This seems a bit excessive, as this is a sanity check on sys.stdout, no?
There was a problem hiding this comment.
@denialhaag You're right—in normal CLI usage, sys.stdout is virtually always going to be a TextIOWrapper, so this is indeed a sanity check rather than defending against likely runtime issues.
A few observations:
- This assert follows the same pattern used in
output.pywhere assertions were added for I/O type checking (noted in the AI summary) - The assert is primarily there to satisfy the
tytype checker rather than to catch real runtime errors - While CodeRabbit's point about
python -Ostripping asserts is technically correct, it's pragmatically unlikely to matter for a CLI tool
If you're comfortable that this is just helping the type checker understand the types and is consistent with your approach in output.py, keeping the assert is reasonable. The scenario where this would actually matter (someone running the CLI with -O flag and having a non-text stdout) is pretty contrived.
Your call on whether to keep it as-is or make it more defensive! 👍
🧠 Learnings used
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1337
File: pyproject.toml:351-351
Timestamp: 2025-11-28T14:33:15.199Z
Learning: In the munich-quantum-toolkit/core repository, the dev dependency "ty==0.0.1a27" is intentionally pinned to an exact pre-release version. This dependency is used by the `ty-check` pre-commit hook (via `uv run --only-dev ty check`), and the exact pin ensures all developers run the same version, preventing unexpected issues from updates since ty is in alpha and changing rapidly.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_bench.py (1)
886-966: Mirror test:GatesInBasis(target=target_obj)still runs for ALG/INDEP wheretarget_objisNone.Even with the new mapped-only assertion, the loop earlier appears to build a PassManager with
target=Nonefor ALG/INDEP iterations, which is very likely to fail at runtime. Gate-basis validation should be conditional ontarget_obj is not None(or on levels that require a target).Proposed fix
@@ - # Ensure the mirror circuit contains only native gates for the given target. - pm = PassManager(GatesInBasis(target=target_obj)) - pm.run(qc_mirror) - assert pm.property_set["all_gates_in_basis"] + # Ensure the mirror circuit contains only native gates for the given target. + if target_obj is not None: + pm = PassManager(GatesInBasis(target=target_obj)) + pm.run(qc_mirror) + assert pm.property_set["all_gates_in_basis"]
🤖 Fix all issues with AI agents
In @src/mqt/bench/benchmarks/__init__.py:
- Around line 32-36: The comprehension building _DISCOVERED_BENCHMARKS uses
slicing (entry.name[:-3]) to strip the ".py" suffix; replace that slicing with
entry.name.removesuffix(".py") in the set comprehension so module names are
correctly and more clearly derived, keeping the same filters (entry.is_file(),
entry.name.endswith(".py"), and not entry.name.startswith("_")) and the variable
name _DISCOVERED_BENCHMARKS unchanged.
In @src/mqt/bench/cli.py:
- Around line 16-17: The code uses assert for stream type-narrowing which can be
stripped with -O; replace those asserts with typing.cast(TextIOBase, stream)
when you only need a type hint (import cast from typing) or perform a real
runtime check and raise a clear exception (e.g., ValueError) if the stream is
not a TextIOBase so users get a usable error; update the occurrences around the
import and the uses in write_circuit() (and the similar asserts at lines
~138-141) to use either cast(stream, TextIOBase) for type-only narrowing or an
isinstance(...) check + raise with a descriptive message for runtime validation.
In @src/mqt/bench/output.py:
- Around line 18-24: The alias __qiskit_version__ is misleading because from
qiskit.version import __version__ returns only Terra's version string; change
the import to either from qiskit import __version__ and keep the name
__qiskit_version__, or rename the existing import to accurately reflect Terra
(e.g., __terra_version__) and update all references to __qiskit_version__ in
this module (e.g., any uses in output formatting or logs) to the new symbol so
it matches the actual value returned by dump.
In @tests/test_bench.py:
- Around line 281-287: The test currently forces opt_level to 2 by always
passing opt_level=opt_level if opt_level is not None else 2 to get_benchmark,
which prevents exercising the default/omitted-opt_level code path; change the
call to only pass the opt_level keyword when opt_level is not None (i.e., call
get_benchmark with opt_level omitted when opt_level is None) so the function’s
default behavior is covered by the test and keep the other args (benchmark,
level, circuit_size, target) unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
pyproject.tomlsrc/mqt/bench/benchmark_generation.pysrc/mqt/bench/benchmarks/__init__.pysrc/mqt/bench/benchmarks/shor.pysrc/mqt/bench/cli.pysrc/mqt/bench/output.pytests/test_bench.py
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1337
File: pyproject.toml:351-351
Timestamp: 2025-11-28T14:33:15.199Z
Learning: In the munich-quantum-toolkit/core repository, the dev dependency "ty==0.0.1a27" is intentionally pinned to an exact pre-release version. This dependency is used by the `ty-check` pre-commit hook (via `uv run --only-dev ty check`), and the exact pin ensures all developers run the same version, preventing unexpected issues from updates since ty is in alpha and changing rapidly.
📚 Learning: 2025-11-04T14:26:25.420Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:11-19
Timestamp: 2025-11-04T14:26:25.420Z
Learning: In the munich-quantum-toolkit/core repository, Qiskit is always available as a dependency during testing, so import guards for qiskit-dependent imports in test files (e.g., test/python/qdmi/qiskit/*.py) are not necessary.
Applied to files:
src/mqt/bench/benchmarks/__init__.py
📚 Learning: 2025-12-21T22:35:08.572Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/fomac/fomac.cpp:348-364
Timestamp: 2025-12-21T22:35:08.572Z
Learning: In the munich-quantum-toolkit/core repository's nanobind bindings, use `.sig("...")` on parameter arguments that have vector or container defaults (e.g., `"sites"_a.sig("...") = std::vector<fomac::Session::Device::Site>{}`) to prevent exposing mutable defaults in the Python API, which would be flagged as a code smell by Python linters. This pattern is preferred over removing `.sig("...")` even though it shows `...` in the stub signature.
Applied to files:
src/mqt/bench/benchmarks/shor.pysrc/mqt/bench/benchmark_generation.py
📚 Learning: 2025-11-26T10:29:36.435Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/fomac/test_fomac.py:54-66
Timestamp: 2025-11-26T10:29:36.435Z
Learning: In test/python/fomac/test_fomac.py, the `ddsim_device` fixture returns `Device | None` even though it only returns a `Device` or calls `pytest.skip()`. The `| None` annotation is required for the type checker to pass, as type checkers may not recognize `pytest.skip()` as a function that never returns.
Applied to files:
tests/test_bench.py
📚 Learning: 2026-01-10T18:49:44.352Z
Learnt from: li-mingbao
Repo: munich-quantum-toolkit/core PR: 1396
File: mlir/lib/Conversion/QCOToQC/QCOToQC.cpp:1045-1119
Timestamp: 2026-01-10T18:49:44.352Z
Learning: The QCOToQC conversion pass (mlir/lib/Conversion/QCOToQC/QCOToQC.cpp) does not need defensive mixed-type checks in its func conversion patterns (ConvertQCOFuncFuncOp, ConvertQCOFuncCallOp, ConvertQCOFuncReturnOp) because the conversion workflow always starts from QC to QCO, and the QCToQCO pass already enforces through its dynamic legality checks that func operations contain only qubit types (no mixed classical/quantum). This upstream guarantee justifies the all-qubit assumptions in QCOToQC patterns.
Applied to files:
src/mqt/bench/benchmark_generation.py
📚 Learning: 2025-11-28T14:33:15.199Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1337
File: pyproject.toml:351-351
Timestamp: 2025-11-28T14:33:15.199Z
Learning: In the munich-quantum-toolkit/core repository, the dev dependency "ty==0.0.1a27" is intentionally pinned to an exact pre-release version. This dependency is used by the `ty-check` pre-commit hook (via `uv run --only-dev ty check`), and the exact pin ensures all developers run the same version, preventing unexpected issues from updates since ty is in alpha and changing rapidly.
Applied to files:
pyproject.toml
📚 Learning: 2025-12-13T20:08:45.549Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qmap PR: 862
File: pyproject.toml:65-66
Timestamp: 2025-12-13T20:08:45.549Z
Learning: In the qmap project (pyproject.toml), maintain broad compatibility with dependencies across supported Python versions. Avoid artificially raising minimum version requirements unless there's a specific need (e.g., to guarantee binary wheel availability for certain Python versions, or to access required features). The goal is to keep the software as broadly compatible as possible with the rest of the ecosystem.
Applied to files:
pyproject.toml
📚 Learning: 2026-01-09T17:58:10.350Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qcec PR: 817
File: pyproject.toml:81-82
Timestamp: 2026-01-09T17:58:10.350Z
Learning: In the Munich Quantum Toolkit projects using nanobind, setting `wheel.py-api = "cp312"` in `[tool.scikit-build]` enables Stable ABI wheels only for Python 3.12+ (where nanobind supports it), while automatically building regular non-ABI3 wheels for earlier Python versions (3.10, 3.11) and free-threading builds (3.14t). This allows a single configuration to appropriately handle both old and new Python versions without forcing incompatible ABI requirements.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-04T15:22:19.558Z
Learnt from: marcelwa
Repo: munich-quantum-toolkit/core PR: 1243
File: test/python/qdmi/qiskit/conftest.py:155-157
Timestamp: 2025-11-04T15:22:19.558Z
Learning: The munich-quantum-toolkit/core repository requires Python 3.10 or later, so Python 3.10+ features (such as `zip(..., strict=...)`, pattern matching, etc.) are acceptable and should not be flagged as compatibility issues.
Applied to files:
pyproject.toml
📚 Learning: 2026-01-09T00:27:20.419Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 39
File: .pre-commit-config.yaml:135-135
Timestamp: 2026-01-09T00:27:20.419Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, `.pre-commit-config.yaml` intentionally uses `https://github.com/adhtruong/mirrors-typos` instead of the official `https://github.com/crate-ci/typos` repository to avoid pre-commit autoupdate tag confusion described in https://github.com/crate-ci/typos/issues/390.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-27T21:22:11.330Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qmap PR: 846
File: pyproject.toml:213-214
Timestamp: 2025-11-27T21:22:11.330Z
Learning: In the qmap project (pyproject.toml), the maintainers intentionally enable `unsafe-fixes = true` in the ruff configuration. This is an established practice that has worked well for the project, with the expectation that PR authors review any automated fixes.
Applied to files:
pyproject.toml
📚 Learning: 2025-11-24T10:19:41.147Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/core PR: 1326
File: python/mqt/core/__init__.py:22-22
Timestamp: 2025-11-24T10:19:41.147Z
Learning: In the munich-quantum-toolkit/core repository, Ruff is configured with 'ALL' rules enabled by default, and only specific rules are selectively disabled. When reviewing changes that enable previously-disabled rules (like PLC0415), noqa directives for those rules become necessary and should be retained.
Applied to files:
pyproject.toml
📚 Learning: 2025-12-14T15:23:54.712Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 23
File: docs/conf.py:110-130
Timestamp: 2025-12-14T15:23:54.712Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, the Ruff configuration has 'ALL' rules enabled with only specific rules disabled. PLR6301 (no-self-use) is active, so `# noqa: PLR6301` directives are necessary for methods that don't use self, even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
pyproject.toml
📚 Learning: 2026-01-08T10:07:32.871Z
Learnt from: flowerthrower
Repo: munich-quantum-toolkit/core-plugins-catalyst PR: 20
File: python/mqt/core/plugins/catalyst/device.py:69-69
Timestamp: 2026-01-08T10:07:32.871Z
Learning: In the munich-quantum-toolkit/core-plugins-catalyst repository, Ruff is configured with select = ["ALL"] and only specific rules are disabled. SLF001 (flake8-self - private member access) is enabled, so `# noqa: SLF001` directives are necessary when accessing private attributes like `device._to_matrix_ops` in python/mqt/core/plugins/catalyst/device.py, even if Ruff reports RUF100 warnings suggesting the directive is unused.
Applied to files:
pyproject.toml
📚 Learning: 2026-01-09T16:51:20.555Z
Learnt from: burgholzer
Repo: munich-quantum-toolkit/qcec PR: 816
File: python/mqt/qcec/__init__.py:20-20
Timestamp: 2026-01-09T16:51:20.555Z
Learning: In the munich-quantum-toolkit/qcec repository, RUF067 (non-empty-init-module) is a Ruff preview rule introduced in version 0.14.11 that flags non-empty __init__.py files. The `# noqa: RUF067` directive in python/mqt/qcec/__init__.py is necessary and correct to suppress warnings for legitimate code (like the Windows DLL patch).
Applied to files:
pyproject.toml
🧬 Code graph analysis (2)
tests/test_bench.py (1)
src/mqt/bench/output.py (2)
generate_header(57-103)OutputFormat(34-43)
src/mqt/bench/benchmark_generation.py (1)
src/mqt/bench/configuration_options.py (1)
ConfigurationOptions(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
🔇 Additional comments (21)
src/mqt/bench/benchmarks/shor.py (4)
156-157: LGTM!Correct fix for the in-place compose pattern. Since
compose(inplace=True)returnsNonein Qiskit, explicitly returning the circuit object ensures the return type matches theQuantumCircuitannotation.
206-207: LGTM!Consistent application of the in-place compose pattern with explicit return.
209-210: LGTM!The return type annotation is now correct. The function returns
circuit(aQuantumCircuit), not anInstruction. This fixes the type inconsistency caught by ty.
300-301: LGTM!Consistent application of the in-place compose pattern with explicit return. The caller at line 76 correctly uses the returned
QuantumCircuit.pyproject.toml (3)
135-139: Coveragerun.disable_warningsaddition looks fine.
no-sysmonis a sensible suppression if you’re hitting the known sysmon warning noise on some platforms.
262-269: Ty config is correctly set up for ty 0.0.11. The configuration uses valid keys and syntax:[tool.ty.terminal].error-on-warningaccepts boolean values (supported), and[tool.ty.src].excludecorrectly uses gitignore-style glob patterns like"docs/**"(supported).
77-82: No issues found.ty==0.0.11is a published version with supported pyproject.toml configuration sections ([tool.ty.terminal],[tool.ty.src]); exact version pinning aligns with the organization's established practice for alpha toolchain consistency.src/mqt/bench/benchmarks/__init__.py (1)
71-74: Dynamic import uses the correct module base (__name__).tests/test_bench.py (4)
542-546: UsingBenchmarkLevel.INDEPin header tests is the right direction.
1014-1026: Deduplicating the benchmark name in the duplicate-registration test is cleaner.
1046-1049:benchmark: strparametrize typing fix looks correct.
185-193: No issue found; test is correctly designed. The test only passeskindas a keyword argument to the three benchmarks that accept it (cdkm_ripple_carry_adder, draper_qft_adder, vbe_ripple_carry_adder). All other test cases havekind=None, so theif kind:check prevents the parameter from being passed to benchmarks that don't accept it.Likely an incorrect or invalid review comment.
src/mqt/bench/output.py (2)
117-135: Text/Binary stream typing viaTextIOBase | BinaryIOis a solid improvement.
149-171: Runtime stream-mode enforcement for QASM vs QPY looks correct and matches the tests.src/mqt/bench/benchmark_generation.py (7)
137-138: LGTM! Defensive null check prevents potential AttributeError.The additional check ensures
target_qc.layoutis notNoneafter transpilation before attempting to setinitial_layout. Sincetranspile()returns a new circuit object, its layout state may differ from the original.
189-198: LGTM! Union overload supports callers with mixed-type variables.This overload enables type-safe calls when the
benchmarkargument is typed asstr | QuantumCircuitrather than a concrete type. The pattern of having specific overloads plus a union fallback is appropriate for this use case.
250-260: LGTM!Consistent application of the union overload pattern for
get_benchmark_indep.
319-330: LGTM!Consistent application of the union overload pattern for
get_benchmark_native_gates.
419-430: LGTM!Consistent application of the union overload pattern for
get_benchmark_mapped.
504-516: LGTM!Consistent application of the union overload pattern for the main
get_benchmarkdispatcher function.
375-376: The type ignore comment is justified—Qiskit's stubs don't useLiteraloverloads to narrow return types.The
remove_final_measurements(inplace=False)method returnsQuantumCircuit(confirmed by Qiskit docs), but the type stubs likely lack@overloaddefinitions withLiteral[False]to properly narrow the return type. The type checker sees a union return and cannot infer thatinplace=FalseguaranteesQuantumCircuit, so the# ty: ignore[invalid-argument-type]suppresses a false positive caused by inadequate type annotations in Qiskit.
| from typing import TYPE_CHECKING, overload | ||
|
|
||
| from qiskit import __version__ as __qiskit_version__ | ||
| from qiskit.qasm2 import dump as dump2 | ||
| from qiskit.qasm3 import dump as dump3 | ||
| from qiskit.qpy import dump as dump_qpy | ||
| from qiskit.version import __version__ as __qiskit_version__ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
qiskit.version __version__ stable API supported versions
💡 Result:
Short answer and commands
- To print the installed Qiskit package/component versions:
- import qiskit; print(qiskit.qiskit_version) # dict of component versions. [1][2]
- Notes about version:
- qiskit.version (or qiskit.version.version) historically reports the terra package version (namespace provided by qiskit-terra), so it may not reflect the full SDK meta-package. Use qiskit_version for all components. [2][3]
- Stable API / versioning policy:
- Qiskit uses semantic versioning and treats documented, non-private symbols as the public API; the 1.x series is promised to maintain backwards compatibility (with documented exceptions). Qiskit maintains an extended support window for previous major series to ease migrations. See the Qiskit SDK version strategy/release notes. [4][5]
- Python / platform support guidance:
- Qiskit removed Python 3.7 support in recent releases (>=0.44) and has progressively deprecated older Python versions; check the release notes for the specific Qiskit release you plan to use for exact supported Python/minimum platform versions. [6][7]
Sources
[1] IBM Qiskit docs — Install Qiskit / checking versions.
[2] Qiskit / community notes and Stack Exchange discussion on qiskit_version vs version.
[3] Tutorials and posts explaining qiskit.version returns terra version.
[4] Qiskit SDK version strategy / policy (semantic versioning, public API definition).
[5] Qiskit 1.0 release notes (stability policy for 1.x).
[6] Qiskit release notes (0.43/0.44) — Python 3.7 removal.
[7] Qiskit SDK release notes & component release pages (see specific package release notes for exact supported Python/platform ranges).
If you want, I can fetch the exact lines from the release notes for a particular Qiskit release (e.g., 1.3.x or 0.46.x) and list the supported Python versions and the output examples for the version commands.
🏁 Script executed:
# Find version requirements and dependencies
find . -type f \( -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" -o -name "requirements*.txt" \) | head -20Repository: munich-quantum-toolkit/bench
Length of output: 88
🏁 Script executed:
# Check how __qiskit_version__ is used in the codebase
rg "__qiskit_version__" -A 2 -B 2Repository: munich-quantum-toolkit/bench
Length of output: 1303
🏁 Script executed:
cat pyproject.toml | head -100Repository: munich-quantum-toolkit/bench
Length of output: 3144
Clarify the import naming: __version__ reports only the Qiskit Terra version, not the full SDK version.
The import from qiskit.version import __version__ is stable and supported across your minimum version (Qiskit ≥1.3.2), but the alias __qiskit_version__ is misleading. The name suggests the full Qiskit SDK version dict, but this import actually retrieves only the Terra component version (a string). Consider either (1) importing from qiskit directly as from qiskit import __version__ for clarity, or (2) updating the variable name to something like __terra_version__ to accurately reflect what it contains.
🤖 Prompt for AI Agents
In @src/mqt/bench/output.py around lines 18 - 24, The alias __qiskit_version__
is misleading because from qiskit.version import __version__ returns only
Terra's version string; change the import to either from qiskit import
__version__ and keep the name __qiskit_version__, or rename the existing import
to accurately reflect Terra (e.g., __terra_version__) and update all references
to __qiskit_version__ in this module (e.g., any uses in output formatting or
logs) to the new symbol so it matches the actual value returned by dump.
| qc = get_benchmark( | ||
| benchmark_name, | ||
| level, | ||
| circuit_size, | ||
| target, | ||
| opt_level, | ||
| benchmark=benchmark_name, | ||
| level=level, | ||
| circuit_size=circuit_size, | ||
| target=target, | ||
| opt_level=opt_level if opt_level is not None else 2, | ||
| ) |
There was a problem hiding this comment.
Keyword-arg switch is fine, but consider preserving coverage for default opt_level behavior.
Right now opt_level is always forced to 2 when param is None, so the test no longer exercises the “caller omitted opt_level” path (if that’s meaningful).
🤖 Prompt for AI Agents
In @tests/test_bench.py around lines 281 - 287, The test currently forces
opt_level to 2 by always passing opt_level=opt_level if opt_level is not None
else 2 to get_benchmark, which prevents exercising the default/omitted-opt_level
code path; change the call to only pass the opt_level keyword when opt_level is
not None (i.e., call get_benchmark with opt_level omitted when opt_level is
None) so the function’s default behavior is covered by the test and keep the
other args (benchmark, level, circuit_size, target) unchanged.
Description
This PR replaces
mypywithtyas the static type checker in MQT Bench.In turn, it fixes all the warnings that
tybrought up and the typing errors it found.Checklist: