Repository navigation
✨ Add Qiskit compiler target conversion - #2655
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
90a9aad to
48cff49
Compare
simon1hofmann
added
feature
|
@coderabbitai full review |
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (13)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughQiskit targets and backends can now be converted to ChangesQiskit Target Conversion
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CompilerTarget
participant exportCircuit
participant NativeTranslation
participant NativeCircuitWriter
CompilerTarget->>exportCircuit: target argument
exportCircuit->>NativeTranslation: createCircuit with target
NativeTranslation->>NativeCircuitWriter: construct writer with target
NativeCircuitWriter->>NativeCircuitWriter: match gate using target operation and qubit sites
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete merge-blocking issue is established. The conversion and target-aware export changes appear mergeable subject to normal checks; successful export does not guarantee device execution support. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The examined conversion and export paths retain structural validation and select only known gate implementations. No introduced security issue was demonstrated. Concurrent modification during conversion and security boundaries of applications embedding these APIs remain unestablished. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 12 files. (1 skipped: 1 unsupported.)
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. A rabbit checks the gate names bright, Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
This already looks pretty good to me! Thanks, @simon1hofmann! 🙂
I only have one comment that I would be curious to hear your opinion on:
denialhaag
left a comment
denialhaag
left a comment
There was a problem hiding this comment.
LGTM now! I'll leave the final approval to @burgholzer! 😎
(You may ignore and resolve the nitpick below. 🤠)
|
Let me try to get a quick review in here. Don't want to be the blocker here. |
🤖 *AI text below* 🤖 Centralize structural Target and BackendV2 snapshots while preserving ordered operation applicability and rejecting unrepresentable constraints. Document compiled-output export as the capability check for consumers. Assisted-by: Codex
🤖 *AI text below* 🤖 Reuse native gate recognition and retain backend aliases through target serialization and Qiskit export. Omit unsupported extras by default while keeping explicit operation selection strict. Move target regressions to the MLIR tests and fix the API documentation reference. Assisted-by: Codex
🤖 *AI text below* 🤖 Pass the optional canonical name by const reference through both target constructors and their bindings. Normalize the selected string directly without making an extra value_or copy. Assisted-by: Codex
Use Qiskit target matching for unrestricted parameters and reject explicit selections without native applicability. Exclude controlled import mappings that the compiler cannot retain as native operations. Keep legacy standard gate export without custom instruction wrappers or canonical-name schema overrides. Validated 579 Python, 55 native, and 104 downstream integration tests, stub generation, repository lint, whole-file C++ lint, and the full documentation build with link checks. Assisted-by: Codex
🤖 *AI text below* 🤖 Return an owned CompilerTarget from the versioned Qiskit adapter and keep gate recognition private. Retain the public Python wrapper for backend normalization and default naming, and reuse existing target validation. Preserve selection, warnings, parameter constraints, and target snapshots. Add direct binding and warning regressions and regenerate the stubs. Validated 581 Python tests, repository lint, full-file C++ lint, stub generation, and the full executable documentation build with local link checks. The two Garnet tests passed when rerun with the existing local device library. Assisted-by: Codex
🤖 *AI text below* 🤖 Separate the summary from the details with a blank line in the native binding and regenerate the Python stub. Validated stub generation, the runtime docstring, repository lint, full-file C++ lint, and the full docs build with local link checks. Assisted-by: Codex
🤖 *AI text below* 🤖 Accept Target and BackendV2 directly in the native adapter and expose CompilerTarget.from_qiskit. Remove the Python forwarding wrapper. Document operation selection, target mapping, and layout behavior. Simplify the Qiskit guide and avoid alias scans for unrelated gates. Preserve the layout metadata contract from merged PR #2553. Assisted-by: Codex
09ff676 to
80a54e8
Compare
burgholzer
left a comment
burgholzer
left a comment
There was a problem hiding this comment.
Alright. This is looking pretty good now as well. I folded the layout changes into here. I'll merge once CI passes to also get the other PR along 🚀
🤖 AI text below 🤖
Description
Add
CompilerTarget.from_qiskit()inmqt.core.mlirto snapshot a QiskitTargetorBackendV2directly in the native adapter. A backend supplies thedefault target name;
namecan override it. The Python forwarding wrapper isremoved.
The snapshot retains supported standard gates, measurement, reset, ordered
operation sites, and undirected routing connectivity. Parameter slots must be
unrestricted. Unrepresentable gates warn and are omitted by default; an
explicit
operation_namesselection rejects them. Instructions without nativegate applicability are skipped by default. Calibration, timing, and scheduling
data are not copied. The target needs a known positive qubit count and connected
topology.
Target-aware
to_qiskit()selects applicable standard gate names, includinglegacy
u1andu3, while preserving ordered gate placements. The docs explainthe structural snapshot, synthesis limits, export checks, and its interaction
with the layout metadata added by #2553. Successful export does not establish
device execution support.
No new dependencies are required.
Validation
c4fb54f4f(main, including ✨ Retain compiler layout metadata #2553).documentation link checks passed locally. The documentation build suppressed
existing notebook lexer warnings.
Checklist
If PR contains AI-assisted content:
🤖 *AI text below* 🤖(titles are exempt).Codex assisted with implementation, tests, validation, review, and this description.