feat: add qbraid_qir.qiskit module for Qiskit to QIR conversion#271
Merged
Conversation
Port of microsoft/qiskit-qir (MIT License) with the following updates: - Compatible with Qiskit 2.x (new QuantumCircuit API) - Uses qiskit_to_qir() interface to match cirq_to_qir/qasm3_to_qir - Supports pyqir 0.10-0.12+ (typed and opaque pointers) - Follows qbraid-qir conventions and code style New files: - qbraid_qir/qiskit/ module with convert, visitor, elements, exceptions - tests/qiskit_qir/ with 44 unit tests - NOTICE.md for MIT license attribution - docs/api/qbraid_qir.qiskit.rst Updated files: - pyproject.toml: added qiskit extra - tox.ini: added qiskit to test environments - README.md: added qiskit usage example - docs/index.rst: added qiskit to API reference - docs/conf.py: added qiskit to autodoc_mock_imports
Follow the pattern from qBraid SDK (azure/result_builder.py): - Keep Apache 2.0 as the primary license - Add pylint comments and attribution block referencing microsoft/qiskit-qir - Reference NOTICE.md for full MIT license text
Contributor
Author
|
CI workflows may need approval since this is from a fork. I've updated the license headers to follow the pattern from the qBraid SDK (
All local tests pass (225 passed, 5 skipped). Docs build successfully. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a qbraid_qir.qiskit subpackage: qiskit_to_qir conversion API, element modeling, mapping tables, BasicQiskitVisitor translator, QiskitConversionError, package exports, docs/NOTICE/packaging/CI updates, and extensive tests. ChangesQiskit conversion
sequenceDiagram
actor User
participant Circuit as Qiskit QuantumCircuit
participant Convert as qiskit_to_qir
participant QModule as QiskitModule
participant Visitor as BasicQiskitVisitor
participant PyQIR as pyqir.Module
User->>Convert: qiskit_to_qir(circuit, name?, transpile?, **kwargs)
Convert->>Convert: validate type & non-empty
Convert->>Circuit: optionally transpile to QISKIT_BASIS_GATES
Convert->>PyQIR: create Context & pyqir.Module(name)
Convert->>QModule: QiskitModule.from_circuit(circuit, module)
QModule->>QModule: parse registers & instructions -> elements
Convert->>Visitor: instantiate with kwargs
Visitor->>QModule: visit_qiskit_module(qiskit_module)
Visitor->>Visitor: visit_register / visit_instruction for each element
Visitor->>PyQIR: emit QIR bodies, runtime calls, record_output, return
Visitor->>Convert: built module
Convert->>PyQIR: verify module
Convert-->>User: return verified pyqir.Module
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 19: Update the CHANGELOG entry for the qbraid_qir.qiskit addition to
reflect the actual function signature of qiskit_to_qir by noting that the name
parameter is optional (defaults to None) rather than required; locate the entry
referencing qiskit_to_qir(circuit, name, **kwargs) and change the description to
show qiskit_to_qir(circuit, name=None, **kwargs) or explicitly state "name is
optional (defaults to None)" so the documentation matches the implementation.
In `@docs/api/qbraid_qir.qiskit.rst`:
- Around line 6-8: The automodule directives (e.g., the one for
qbraid_qir.qiskit) are missing the :members: option so module members
(classes/functions) are not included; update each API rst
(qbraid_qir.qiskit.rst, qbraid_qir.cirq.rst, qbraid_qir.qasm3.rst,
qbraid_qir.squin.rst) to add the :members: option alongside :undoc-members: and
:show-inheritance: in the automodule directive to ensure all module members are
documented.
In `@NOTICE.md`:
- Around line 33-37: NOTICE.md currently does not end with exactly one trailing
newline; update the file to ensure it finishes with a single newline character
(no extra blank lines or missing newline) so it satisfies markdownlint
MD047—open NOTICE.md, remove any extra trailing blank lines if present, and add
exactly one '\n' at EOF.
- Around line 1-3: Update the header and phrasing to use hyphenated
“Third-Party” consistently: change the heading text "Third Party Notices" to
"Third-Party Notices" and change "third party sources" to "third-party sources"
so both the title and body use the hyphenated form.
In `@pyproject.toml`:
- Line 43: Update the Qiskit extra dependency specification in pyproject.toml
(the line currently reading qiskit = ["qiskit>=2.0"]) to constrain to the 2.x
series so future major releases won’t be pulled in; replace the loose ">=2.0"
spec with a 2.x‑bounded spec (e.g., >=2.0,<3.0) to pin the major version while
allowing patch/minor updates.
In `@qbraid_qir/__init__.py`:
- Line 57: _lazy currently maps backends to plain strings which are iterated as
character sequences by __dir__ and __getattr__; change the values in _lazy to
sequences (e.g., tuples or lists) of object/module names (e.g., ("cirq_to_qir",)
not "cirq_to_qir") so downstream logic treats each entry as a single name.
Update the _lazy definition and ensure any code that iterates _lazy (notably
__dir__ and __getattr__) expects sequences of names rather than characters; keep
the same identifiers "cirq", "qasm3", "qiskit" as keys and use singleton
tuples/lists as their values.
In `@qbraid_qir/qiskit/__init__.py`:
- Line 59: The __all__ export list is unsorted which triggers lint churn; in
qiskit/__init__.py, sort the names in the __all__ list alphabetically (e.g.,
"BasicQiskitVisitor", "QiskitConversionError", "QiskitModule", "qiskit_to_qir")
so the exported symbols BasicQiskitVisitor, QiskitConversionError, QiskitModule
and qiskit_to_qir appear in lexicographic order.
In `@qbraid_qir/qiskit/convert.py`:
- Line 36: The public entry function qiskit_to_qir currently uses an untyped
**kwargs; change its signature to document and type the visitor/visitor-related
kwargs (e.g., add a typed keyword-only parameter or type the var-kwargs). For
example, import typing (Any, Optional, Mapping) and replace **kwargs with a
typed parameter such as *, visitors: Optional[Mapping[str, Any]] = None (or keep
**kwargs: Any if you must accept arbitrary keys) and update all internal uses of
kwargs inside qiskit_to_qir to reference the new name (visitors or
visitor_kwargs) so callers and static checkers get proper hints.
In `@qbraid_qir/qiskit/elements.py`:
- Around line 38-49: The base class method
_QuantumCircuitElement.from_element_list creates instances via cls(elem), which
is incompatible with _Instruction.__init__(instruction, qargs, cargs); update
the code so _Instruction explicitly overrides from_element_list (or defines a
classmethod) that either raises NotImplementedError to prevent misuse or
implements the correct construction used in from_circuit, and ensure you
reference _Instruction.__init__ and the existing from_circuit logic when
implementing the override so callers won't attempt to use the base
implementation.
- Around line 97-113: The __init__ method in qbraid_qir/qiskit/elements.py
currently takes eight parameters (circuit, name, module, num_qubits, num_clbits,
reg_sizes, elements) which triggers pylint's too-many-arguments error; fix this
by either 1) adding a local suppression for this constructor (add a pylint
disable comment for too-many-arguments on the __init__ method) if the signature
must remain stable, or 2) refactoring the signature to accept a single
config/dataclass object (e.g., ElementsConfig) that contains circuit, name,
module, num_qubits, num_clbits, reg_sizes, elements and then update the
constructor body that assigns to self._circuit, self._name, self._module,
self._elements, self._num_qubits, self._num_clbits, and self.reg_sizes to pull
values from that config; choose one approach and apply consistently to the
__init__ function to resolve the lint failure.
In `@qbraid_qir/qiskit/visitor.py`:
- Around line 238-323: The visit_instruction method has too many branches and
statements; refactor by introducing gate-dispatch tables (e.g.,
_SINGLE_QUBIT_GATES, _MULTI_QUBIT_GATES, _ROTATION_GATES) that map op_name
strings to qis functions, then replace the long if/elif chain in
visit_instruction with lookups that call the mapped function (passing
self._builder and either *qubits or *instruction.params, *qubits as
appropriate), while keeping special-case handling for "measure"/"m"/"mz",
"barrier", "id", NOOP_INSTRUCTIONS, and composite instructions
(instruction.definition) unchanged (use visit_instruction,
_process_composite_instruction, and qis.* symbols to locate code to modify).
- Line 185: The debug call uses an unnecessary list comprehension; change the
logger.debug invocation that currently formats "Added labels for qubits %s" with
[bit for bit in register] to instead use list(register) so the expression is
clearer and avoids creating an extra comprehension; locate the logger.debug call
in the visitor (the line that logs "Added labels for qubits %s" and uses
register) and replace the comprehension with list(register).
- Around line 263-265: The loop that pairs qubits and results uses zip(qubits,
results) and can silently drop mismatched items; change it to use zip(qubits,
results, strict=True) so mismatched lengths raise an error, and keep the
existing body that sets self._measured_qubits[pointer_id(qubit)] = True and
calls qis.mz(self._builder, qubit, result) to preserve behavior; update the loop
in visitor.py where pointer_id, self._measured_qubits, qis.mz, and self._builder
are referenced.
- Around line 31-32: The imports use module-qualified import style causing a
pylint violation; change the two import statements in qbraid_qir.qiskit.visitor
(currently "import pyqir.qis as qis" and "import pyqir.rt as rt") to the package
import form "from pyqir import qis" and "from pyqir import rt" so the module
symbols qis and rt are imported via the package namespace and satisfy the
linter.
- Around line 86-112: The BasicQiskitVisitor class currently triggers pylint's
too-many-instance-attributes (10/7); to silence this CI error, add a local
pylint pragma that disables too-many-instance-attributes for this class: place a
"# pylint: disable=too-many-instance-attributes" comment immediately before the
class BasicQiskitVisitor declaration (so it covers __init__ and attributes like
_module, _qiskit_module, _builder, _entry_point, _qubit_labels, _clbit_labels,
_measured_qubits, _initialize_runtime, _record_output, _emit_barrier_calls) and
then re-enable the check with "# pylint: enable=too-many-instance-attributes"
immediately after the class definition (or at the end of the file) to limit the
suppression scope.
In `@tests/qiskit_qir/conftest.py`:
- Line 24: Replace the unnecessary empty-call fixture decorators: change every
occurrence of the decorator `@pytest.fixture`() to the bare `@pytest.fixture` for
each fixture in conftest.py (all 13 fixtures that currently use
`@pytest.fixture`() — e.g., the decorators on the fixtures at the noted
locations), so the fixtures use the bare decorator form and satisfy PT001.
In `@tests/qiskit_qir/test_basic_gates.py`:
- Around line 104-107: The current assertion is too permissive because "double"
can appear elsewhere; instead, locate the function IR for
"__quantum__qis__{gate_name}__body" (use the existing ir = str(module) and the
f"__quantum__qis__{gate_name}__body" check), then assert that a precise angle
literal appears in that function body (e.g., look inside the function substring
and require a specific LLVM constant/token such as "double 0.5" or the exact
encoding you expect like "0.5" or "5.0" within that function body) rather than
asserting "double" anywhere in the whole IR. Ensure you reference ir, module and
gate_name when extracting the function substring and replace the broad assertion
with this targeted check.
- Line 57: Replace comma-separated string argnames in the
pytest.mark.parametrize decorators with a tuple of strings to satisfy PT006:
change occurrences like pytest.mark.parametrize("gate_name,qir_name",
SINGLE_QUBIT_GATES.items()) to pytest.mark.parametrize(("gate_name",
"qir_name"), SINGLE_QUBIT_GATES.items()) (also apply the same tuple-style fix
for the other parametrize uses at the same file around the other decorators on
lines where parametrize is used).
In `@tests/qiskit_qir/test_qiskit_to_qir.py`:
- Line 191: Replace the chained or equality in the test assertion by using a
membership check: update the assertion that currently compares module.name ==
bell_circuit.name or module.name == "main" to assert that module.name is in a
collection containing bell_circuit.name and "main" (referencing module.name and
bell_circuit.name in the test_qiskit_to_qir.py assertion).
- Line 167: Update the pytest.raises call in test_qiskit_to_qir.py that
currently uses match="must be of type qiskit.QuantumCircuit" to use a raw,
escaped regex so the dots are treated literally; change the match argument to a
raw string with escaped dots (e.g., r"must be of type qiskit\.QuantumCircuit")
in the pytest.raises(...) invocation to prevent unintended matches for
any-character dots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f043602-6154-4b38-9018-e50948ff1ef1
📒 Files selected for processing (18)
CHANGELOG.mdNOTICE.mdREADME.mddocs/api/qbraid_qir.qiskit.rstdocs/conf.pydocs/index.rstpyproject.tomlqbraid_qir/__init__.pyqbraid_qir/qiskit/__init__.pyqbraid_qir/qiskit/convert.pyqbraid_qir/qiskit/elements.pyqbraid_qir/qiskit/exceptions.pyqbraid_qir/qiskit/visitor.pytests/qiskit_qir/__init__.pytests/qiskit_qir/conftest.pytests/qiskit_qir/test_basic_gates.pytests/qiskit_qir/test_qiskit_to_qir.pytox.ini
…flow The CI was failing for two reasons: 1. Pylint violations in the new qiskit module (too-many-arguments, too-many-instance-attributes, too-many-branches, too-many-statements, consider-using-from-import, unnecessary-comprehension, consider-using-in) 2. Missing `--extra qiskit` in the test workflow, causing ModuleNotFoundError for qiskit during test collection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Pushed a fix for the CI failures in b69c5d9. There were two issues: 1. Pylint violations (format-check)
All resolved with targeted fixes and pylint disable comments consistent with the existing codebase patterns (e.g. 2. Missing qiskit dependency in test workflow Verified locally — pylint scores 10/10 and all 43 qiskit tests pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/qiskit_qir/test_qiskit_to_qir.py (1)
167-167:⚠️ Potential issue | 🟡 MinorEscape the regex in
pytest.raisesmatch.Line 167 uses a non-raw pattern with unescaped
.characters, so the assertion can match unintended messages.🔧 Proposed fix
- with pytest.raises(TypeError, match="must be of type qiskit.QuantumCircuit"): + with pytest.raises(TypeError, match=r"must be of type qiskit\.QuantumCircuit"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/qiskit_qir/test_qiskit_to_qir.py` at line 167, The test's pytest.raises call uses an unescaped regex for the match string ("must be of type qiskit.QuantumCircuit") so the dots match any character; update the assertion in the test (the pytest.raises invocation around the failing call) to use a raw, escaped regex for the exact type name (e.g., use a raw string with escaped dots such as r"must be of type qiskit\.QuantumCircuit" or build the pattern with re.escape("qiskit.QuantumCircuit")) so the error message match is precise.qbraid_qir/qiskit/visitor.py (1)
264-266:⚠️ Potential issue | 🟡 MinorUse strict zip for measurement pairing.
Line 264 should fail fast on mismatched qubit/result lengths instead of silently dropping items.
🔧 Proposed fix
- for qubit, result in zip(qubits, results): + for qubit, result in zip(qubits, results, strict=True):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qbraid_qir/qiskit/visitor.py` around lines 264 - 266, The measurement loop currently uses zip(qubits, results) which silently drops pairs when lengths differ; update the code around the loop that sets self._measured_qubits and calls qis.mz to validate qubits and results lengths first (e.g. check len(qubits) == len(results) and raise a clear ValueError if not) or iterate with itertools.zip_longest and raise on a missing value, then proceed to set self._measured_qubits[pointer_id(qubit)] = True and call qis.mz(self._builder, qubit, result) for each validated pair.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qbraid_qir/qiskit/elements.py`:
- Around line 141-143: from_circuit currently accepts module: Optional[Module]
but passes None downstream causing BasicQiskitVisitor.visit_qiskit_module to
access module.module.context and crash; update QiskitModule.from_circuit to
handle module==None by either constructing a minimal Module instance (so
module.module.context is valid) or by raising a clear ValueError early—modify
the logic in from_circuit (and the call sites around lines where QiskitModule is
created) to ensure module is never None before returning/using the QiskitModule,
and reference QiskitModule.from_circuit and
BasicQiskitVisitor.visit_qiskit_module when making the change.
---
Duplicate comments:
In `@qbraid_qir/qiskit/visitor.py`:
- Around line 264-266: The measurement loop currently uses zip(qubits, results)
which silently drops pairs when lengths differ; update the code around the loop
that sets self._measured_qubits and calls qis.mz to validate qubits and results
lengths first (e.g. check len(qubits) == len(results) and raise a clear
ValueError if not) or iterate with itertools.zip_longest and raise on a missing
value, then proceed to set self._measured_qubits[pointer_id(qubit)] = True and
call qis.mz(self._builder, qubit, result) for each validated pair.
In `@tests/qiskit_qir/test_qiskit_to_qir.py`:
- Line 167: The test's pytest.raises call uses an unescaped regex for the match
string ("must be of type qiskit.QuantumCircuit") so the dots match any
character; update the assertion in the test (the pytest.raises invocation around
the failing call) to use a raw, escaped regex for the exact type name (e.g., use
a raw string with escaped dots such as r"must be of type qiskit\.QuantumCircuit"
or build the pattern with re.escape("qiskit.QuantumCircuit")) so the error
message match is precise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44848ed7-3207-437b-807c-94a4100d62e9
📒 Files selected for processing (4)
.github/workflows/main.ymlqbraid_qir/qiskit/elements.pyqbraid_qir/qiskit/visitor.pytests/qiskit_qir/test_qiskit_to_qir.py
Member
|
@TheGupta2012 FYI: @eidrynbot Is one of my bots that I have working on a few things |
…ag, expand tests - Extract gate mappings into qbraid_qir/qiskit/maps.py (mirrors qasm3/maps.py pattern) - Refactor visitor.py if-else chain to dict-based lookup (~60 lines -> ~15 lines) - Add transpile=bool parameter to qiskit_to_qir() for pre-conversion transpilation - Tighten qiskit version pin to >=2.0,<3.0 - Identity gate is now a true no-op (no QIR emitted) - Rewrite test_basic_gates.py with sequential equivalence checks (47 tests) - Rewrite test_qiskit_to_qir.py with integration, error, visitor, maps, and API tests - Add test_complex_circuits.py: teleportation, Grover, parameterized, custom gates, large circuits, transpilation, edge cases, and 10 random circuit tests - Fix mypy type errors in elements.py and visitor.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
qbraid_qir/qiskit/elements.py (1)
141-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
from_circuitallowsmodule=Nonebut conversion path requires a real PyQIR module.This can produce a runtime failure later in visitor flow. Either instantiate a default module in
from_circuitor raise early whenmoduleis missing.Also applies to: 169-173
🤖 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 `@qbraid_qir/qiskit/elements.py` around lines 141 - 143, The classmethod QiskitModule.from_circuit currently accepts module: Optional[Module] but downstream conversion requires a real PyQIR Module; update from_circuit (and the other overloaded/duplicated variant at the cited block around lines 169-173) to validate module is not None and raise a clear ValueError (e.g., "module must be provided for circuit->PyQIR conversion") if it is None, or alternatively create and pass a default Module instance before invoking the conversion/visitor; locate and change the checks in from_circuit and the other similar function so the conversion path always receives a concrete Module object (refer to the QiskitModule.from_circuit method name and the duplicate variant mentioned).qbraid_qir/qiskit/visitor.py (1)
256-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
zip(..., strict=True)for measurement pair integrity.If qubit/result lengths diverge, current logic truncates silently. Strict zip fails fast and prevents partial emission.
Proposed fix
- for qubit, result in zip(qubits, results): + for qubit, result in zip(qubits, results, strict=True):🤖 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 `@qbraid_qir/qiskit/visitor.py` around lines 256 - 260, The measurement loop currently uses zip(qubits, results) which silently truncates if lengths differ; change the loop to use zip(qubits, results, strict=True) so mismatched qubit/result lists raise immediately. Update the loop that calls pointer_id(qubit), writes to self._measured_qubits, and calls qis.mz(self._builder, qubit, result) to use the strict zip variant to enforce pair integrity.tests/qiskit_qir/test_qiskit_to_qir.py (1)
216-216:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape regex metacharacters in
pytest.raises(..., match=...).Line 216 uses
.as regex wildcards; this can match unintended messages. Use a raw escaped pattern.Proposed fix
- with pytest.raises(TypeError, match="must be of type qiskit.QuantumCircuit"): + with pytest.raises(TypeError, match=r"must be of type qiskit\.QuantumCircuit"):🤖 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 `@tests/qiskit_qir/test_qiskit_to_qir.py` at line 216, The test's pytest.raises uses an unescaped string "must be of type qiskit.QuantumCircuit" so the dots act as regex wildcards; update the assertion to use an escaped raw regex (e.g. match=r"must be of type qiskit\.QuantumCircuit") or build the pattern with re.escape to ensure the literal "qiskit.QuantumCircuit" is matched; change the pytest.raises call in the test_qiskit_to_qir test to use the escaped/raw pattern.
🤖 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 `@qbraid_qir/qiskit/visitor.py`:
- Around line 116-117: Replace the runtime assertion with an explicit exception:
instead of using "assert module.module is not None, ..." raise a ValueError or
RuntimeError when module.module is None so this check always runs; update the
initialization that sets self._module (the code that currently uses assert with
module.module and then self._module = module.module) to perform an if check and
raise (e.g., if module.module is None: raise ValueError("QiskitModule must have
a PyQIR module set")) before assigning self._module.
In `@tests/qiskit_qir/test_basic_gates.py`:
- Line 91: Replace the ambiguous single-letter loop variable l with a clear name
(e.g., line or ln) in all comprehensions and loops that filter or iterate over
IR body strings—specifically update the comprehension that builds gate_lines
(gate_lines = [l for l in body if l.startswith("call void `@__quantum__qis__`")])
and the other similar occurrences flagged in the file to avoid E741; keep the
logic unchanged, only rename the loop variable and update its usages inside the
comprehension/loop.
In `@tests/qiskit_qir/test_complex_circuits.py`:
- Line 40: Replace the ambiguous single-letter loop variable "l" used in
list/set comprehensions and any(...) checks with a clear name like "line" across
the test file (e.g. the comprehension returning [l for l in _get_body(module) if
"qis__" in l]); update every comprehension and any(...) that currently uses "l"
so they use "line" (or another descriptive identifier) to satisfy Ruff E741 and
avoid CI failures—ensure occurrences in all comprehensions and set/list
expressions are changed consistently.
In `@tests/qiskit_qir/test_qiskit_to_qir.py`:
- Line 62: Replace the ambiguous loop variable name "l" used in list
comprehensions (e.g., the comprehension that builds gate_ops from body: gate_ops
= [l for l in body if "qis__" in l]) with a clearer name like "line"; update
every comprehension in this test module that currently uses "l" (including the
similar comprehensions building measurement_ops, barrier_ops, param_ops, etc.)
to use "line" (or another descriptive name) and ensure all references inside the
comprehension and any dependent variables are updated consistently.
---
Duplicate comments:
In `@qbraid_qir/qiskit/elements.py`:
- Around line 141-143: The classmethod QiskitModule.from_circuit currently
accepts module: Optional[Module] but downstream conversion requires a real PyQIR
Module; update from_circuit (and the other overloaded/duplicated variant at the
cited block around lines 169-173) to validate module is not None and raise a
clear ValueError (e.g., "module must be provided for circuit->PyQIR conversion")
if it is None, or alternatively create and pass a default Module instance before
invoking the conversion/visitor; locate and change the checks in from_circuit
and the other similar function so the conversion path always receives a concrete
Module object (refer to the QiskitModule.from_circuit method name and the
duplicate variant mentioned).
In `@qbraid_qir/qiskit/visitor.py`:
- Around line 256-260: The measurement loop currently uses zip(qubits, results)
which silently truncates if lengths differ; change the loop to use zip(qubits,
results, strict=True) so mismatched qubit/result lists raise immediately. Update
the loop that calls pointer_id(qubit), writes to self._measured_qubits, and
calls qis.mz(self._builder, qubit, result) to use the strict zip variant to
enforce pair integrity.
In `@tests/qiskit_qir/test_qiskit_to_qir.py`:
- Line 216: The test's pytest.raises uses an unescaped string "must be of type
qiskit.QuantumCircuit" so the dots act as regex wildcards; update the assertion
to use an escaped raw regex (e.g. match=r"must be of type
qiskit\.QuantumCircuit") or build the pattern with re.escape to ensure the
literal "qiskit.QuantumCircuit" is matched; change the pytest.raises call in the
test_qiskit_to_qir test to use the escaped/raw pattern.
🪄 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: 53295d17-a35a-4fa7-89bd-d16f314ee42d
📒 Files selected for processing (12)
pyproject.tomlqbraid_qir/qiskit/__init__.pyqbraid_qir/qiskit/convert.pyqbraid_qir/qiskit/elements.pyqbraid_qir/qiskit/exceptions.pyqbraid_qir/qiskit/maps.pyqbraid_qir/qiskit/visitor.pytests/qiskit_qir/__init__.pytests/qiskit_qir/conftest.pytests/qiskit_qir/test_basic_gates.pytests/qiskit_qir/test_complex_circuits.pytests/qiskit_qir/test_qiskit_to_qir.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- Fix CHANGELOG signature to show name=None default - Fix _lazy dict values to be tuples so __dir__() doesn't iterate chars - Override from_element_list in _Instruction to raise NotImplementedError - Replace assert with ValueError in visitor.visit_qiskit_module - Add strict=True to zip(qubits, results) in measurement dispatch - Rename ambiguous loop variable l to line in test files (E741) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@qbraid_qir/qiskit/visitor.py`:
- Around line 87-103: The public methods finalize, record_output, ir, and
bitcode (and any other methods that dereference self._module or self._builder)
must be guarded until the visitor has been initialized: add a simple initialized
check (e.g. self._initialized boolean set in __init__ and flipped when the
builder/module are actually created, or explicitly initialize self._module and
self._builder to None and check for None) and in each public method (finalize,
record_output, ir, bitcode, and the other referenced ranges) raise a clear
RuntimeError if the visitor is not initialized instead of letting AttributeError
surface; reference the fields _module and _builder and the methods
finalize/record_output/ir/bitcode when adding these checks so callers get a
deterministic conversion-failure message.
- Around line 104-130: visit_qiskit_module currently replaces the module/builder
but does not clear visitor state, so previously collected labels/measurements
persist; at the start of visit_qiskit_module (before setting
self._module/self._builder) reset/clear the collections self._qubit_labels,
self._clbit_labels, and self._measured_qubits (e.g. reassign to empty
dict/list/set as appropriate) so each QiskitModule visit starts with fresh
label/measurement state; reference visit_qiskit_module, _qubit_labels,
_clbit_labels, and _measured_qubits when making the change.
In `@tests/qiskit_qir/test_complex_circuits.py`:
- Around line 129-132: The test currently only checks that the emitted operation
string contains "double", which can yield false positives for the RX angle;
update the assertion in tests/qiskit_qir/test_complex_circuits.py to assert a
concrete numeric token representing pi/4 appears in the emitted call (e.g.,
check ops[0] contains the numeric literal or token used for pi/4), keeping the
existing checks for "qis__rx__body" and len(ops) == 1; target the ops variable
and the emitted call string ("qis__rx__body") so the test fails if the bound
angle is not the expected pi/4 value.
- Around line 320-323: The test currently drops multiplicity by comparing sets
of transpiled gate name lists (no_names and yes_names); change the assertion to
preserve counts by comparing multisets—e.g., import collections and assert
collections.Counter(no_names) == collections.Counter(yes_names) (or
alternatively assert sorted(no_names) == sorted(yes_names)) so that differing
counts of the same gate names fail the test; update the assertion that currently
reads assert set(no_names) == set(yes_names) to use Counter or sorted
comparison.
In `@tests/qiskit_qir/test_qiskit_to_qir.py`:
- Line 216: The test's pytest.raises match pattern uses unescaped dots so the
regex matches unintended strings; update the match argument used in the
pytest.raises(...) call to be a raw, escaped regex (e.g., use a raw string and
escape the dot in "qiskit\.QuantumCircuit") so it matches the literal
module.class name exactly.
🪄 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: 1a807730-a02e-4243-8973-48395a5ee225
📒 Files selected for processing (8)
.github/workflows/main.ymlCHANGELOG.mdqbraid_qir/__init__.pyqbraid_qir/qiskit/elements.pyqbraid_qir/qiskit/visitor.pytests/qiskit_qir/test_basic_gates.pytests/qiskit_qir/test_complex_circuits.pytests/qiskit_qir/test_qiskit_to_qir.py
- Guard visitor public methods against uninitialized state with RuntimeError - Reset visitor label/measurement state at start of each module visit - Tighten bound-angle assertion to check concrete pi/4 value - Use sorted() instead of set() to preserve gate count multiplicity in test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
qbraid_qir/qiskit/visitor.py (1)
243-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
visit_instruction()before touching builder state.
visit_instruction()still reachesself._module.contextandself._builderbefore any_check_initialized()call. A direct call on a fresh visitor will therefore raiseAttributeErrorinstead of the deterministic initialization error you added elsewhere. Put_check_initialized()at the top of this method too.🤖 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 `@qbraid_qir/qiskit/visitor.py` around lines 243 - 259, The visit_instruction method currently accesses self._module.context and builder state (e.g., self._module.context, self._builder) before verifying initialization; move or add a call to self._check_initialized() at the very start of visit_instruction (before any use of self._module, self._builder, or related state like creating pyqir.qubit/pyqir.result) so the deterministic initialization error is raised instead of AttributeError.
🤖 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 `@qbraid_qir/qiskit/visitor.py`:
- Around line 153-177: The record_output method writes into self._module and
self._builder but uses module.reg_sizes from the passed-in QiskitModule, which
can mismatch the active module; either enforce that the passed module matches
the active one or always use the active module's register sizes. Update
record_output to verify module is identical to self._qiskit_module (or
raise/return) before proceeding, or change the loop to read sizes from
self._qiskit_module.reg_sizes and keep the rest of the logic unchanged
(references: record_output, self._module, self._builder, module.reg_sizes,
self._qiskit_module).
- Around line 65-70: Add parameter and return type annotations to the abstract
methods on QuantumCircuitElementVisitor: change visit_register(self, register)
to visit_register(self, register: Union[QuantumRegister, ClassicalRegister]) ->
None and change visit_instruction(self, instruction, qargs, cargs) to
visit_instruction(self, instruction: Instruction, qargs, cargs) -> None so the
abstract base matches the concrete BasicQiskitVisitor signatures and enables
static typing; keep the existing imports for Union, QuantumRegister,
ClassicalRegister, and Instruction and update only the method signatures in the
QuantumCircuitElementVisitor class.
---
Duplicate comments:
In `@qbraid_qir/qiskit/visitor.py`:
- Around line 243-259: The visit_instruction method currently accesses
self._module.context and builder state (e.g., self._module.context,
self._builder) before verifying initialization; move or add a call to
self._check_initialized() at the very start of visit_instruction (before any use
of self._module, self._builder, or related state like creating
pyqir.qubit/pyqir.result) so the deterministic initialization error is raised
instead of AttributeError.
🪄 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: 0e309d70-409d-4589-8c16-cf2c7a86f356
📒 Files selected for processing (2)
qbraid_qir/qiskit/visitor.pytests/qiskit_qir/test_complex_circuits.py
ryanhill1
requested changes
May 19, 2026
…d module, move NOTICE.md - Add parameter and return type annotations to QuantumCircuitElementVisitor abstract methods - Remove module param from record_output(); use self._qiskit_module instead - Move NOTICE.md to qbraid_qir/qiskit/ where the adapted code lives
ryanhill1
approved these changes
May 20, 2026
TheGupta2012
approved these changes
May 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Port of microsoft/qiskit-qir (MIT License, now archived) into qbraid-qir as
qbraid_qir.qiskit.Closes #272
Changes
New Module:
qbraid_qir.qiskitqiskit_to_qir(circuit, name, **kwargs): Main conversion function matching thecirq_to_qir/qasm3_to_qirinterfaceQiskitModule: Circuit wrapper for QIR conversionBasicQiskitVisitor: Visitor for converting Qiskit operations to QIRQiskitConversionError: Exception for conversion failuresKey Updates from Original qiskit-qir
QuantumCircuitAPI (CircuitInstructioninstead of tuples)Testing
Documentation
qbraid_qir.qiskitNOTICE.mdfor MIT license attributionConfiguration
qiskitoptional dependency in pyproject.tomlLicense Compliance
The original qiskit-qir is MIT licensed. Code derived from it includes proper attribution in file headers (following the pattern from qBraid SDK) and full license text in
NOTICE.md.Related
Summary by CodeRabbit
New Features
Documentation
Tests
Chores