feat: Add initial jeff-qiskitc converter tool - #84
Conversation
Add QiskitC to Jeff converter for straight-line quantum programsAdds the QkCircuit -> jeff direction (qiskitc_to_jeff), mirroring the
|
There was a problem hiding this comment.
Thanks a lot for your interest in contributing to jeff, @aks8134! 🙂 I'm sorry for getting back to you only now.
This is already a nice starting point for the Qiskit conversions! I think we'll still have to iron out some stylistic aspects, but this should all be doable. As you already pointed out yourself, we should start by adding a GitHub Actions workflow that runs your new tests. You can find some thoughts on this in the comments below, together with some additional points I stumbled across while going through your PR.
Please let me know if setting up the workflow causes issues; we can help with that.
| #include "capnp/jeff.capnp.h" | ||
|
|
||
|
|
||
| QkCircuit* jeff_to_qiskitc(jeff::Module::Reader module); |
There was a problem hiding this comment.
It would be nice to add a wrapper that accepts a .jeff file. For reference, we have these functions for translating between jeff and MLIR:
| } | ||
|
|
||
| capnp::MallocMessageBuilder message; | ||
| jeff::Module::Builder module = message.initRoot<jeff::Module>(); |
There was a problem hiding this comment.
module is rendered as a built-in on GitHub, so something like mod would be better.
| jeff::Module::Builder module = message.initRoot<jeff::Module>(); | |
| jeff::Module::Builder mod = message.initRoot<jeff::Module>(); |
| module.setVersion(0); | ||
| module.setVersionMinor(3); | ||
| module.setVersionPatch(0); | ||
| module.setEntrypoint(0); | ||
| module.initStrings(1).set(0, "from_qkcircuit"); |
There was a problem hiding this comment.
It might also make sense to set the tool and its version here.
| set(CMAKE_CXX_STANDARD 17) | ||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
|
|
||
| find_package(CapnProto CONFIG REQUIRED) |
There was a problem hiding this comment.
In jeff-mlir, we fetch Cap'n Proto via FetchContent. Might also be interesting here.
|
Thanks @denialhaag for the detailed review. I plan to make the recommended changes in the following two commits :
I will try to get back as soon as possible. |
… I/O wrappers - Derive jeff format version and tool name/version from generated CMake/schema constants instead of hand-typed literals - Fix jeff_to_qiskitc to respect Module.entrypoint instead of assuming function 0 - Add jeff_file_to_qiskitc/qiskitc_to_jeff_file for reading/writing .jeff files directly, plus a test - Add clang-format/clang-tidy/cmake-format configs matching jeff-mlir's conventions
Builds Qiskit's C API from source (pinned to the 2.5.2 tag) and runs jeff-qiskitc's test suite on ubuntu-24.04, gated behind a paths-filter so it only runs when jeff-qiskitc or the schema changes.
|
Hi @denialhaag, I made the changes that u suggested. Could you let me know if anything should be changed? |
denialhaag
left a comment
There was a problem hiding this comment.
Thanks a lot for addressing all of my comments, @aks8134! 🙂 It's really cool to see the programs round-tripping successfully in the CI! 🥳
Below, you can find another set of comments that you hopefully find helpful to polish your implementation a bit. Most of them are largely stylistic; the functionality of your implementation is already at a nice point! 🙂
| QkCircuit* jeff_to_qiskitc(jeff::Module::Reader mod); | ||
|
|
||
| kj::Array<capnp::word> qiskitc_to_jeff(const QkCircuit* circuit); | ||
|
|
||
| QkCircuit* jeff_file_to_qiskitc(const std::string& path); | ||
|
|
||
| void qiskitc_to_jeff_file(const QkCircuit* circuit, const std::string& path); |
There was a problem hiding this comment.
It would be nice to add Doxygen comments to these.
| exclude: | | ||
| (?x)^( | ||
| impl/cpp/.*/capnp/jeff\.capnp\.(h|c\+\+) | ||
| )$ |
There was a problem hiding this comment.
Is this necessary, given that we are in the tool directory already? 🤔
|
|
||
| namespace { | ||
|
|
||
| constexpr double kPi = 3.14159265358979323846; |
There was a problem hiding this comment.
If requiring C++20 doesn't create any problems (see one of my comments below), let's use std::numbers::pi instead.
There was a problem hiding this comment.
Could you try writing the tests using GoogleTest? It provides infrastructure for asserting and other nice-to-haves.
You can refer to the test configuration jeff-mlir for help, but also don't hesitate to ask us (or an LLM). 🙂
There was a problem hiding this comment.
If this is too much work, don't worry! We can also take care of that in a follow-up.
| jeff::Module::Reader module = reader.getRoot<jeff::Module>(); | ||
|
|
||
| QkCircuit* roundtripped = jeff_to_qiskitc(module); |
There was a problem hiding this comment.
Just to not trip up GitHub's syntax highlighting.
| jeff::Module::Reader module = reader.getRoot<jeff::Module>(); | |
| QkCircuit* roundtripped = jeff_to_qiskitc(module); | |
| jeff::Module::Reader mod = reader.getRoot<jeff::Module>(); | |
| QkCircuit* roundtripped = jeff_to_qiskitc(mod); |
| enable_testing() | ||
| add_subdirectory(tests) |
There was a problem hiding this comment.
Could you guard this similarly to how we do it in jeff-mlir? This ensures that testing is not enabled when your tool is used downstream.
| uint32_t num_values = num_qubits * 2 + num_floats; | ||
| uint32_t num_ops = num_qubits + num_floats + 1; | ||
|
|
||
| jeff::Module::Builder module = message.initRoot<jeff::Module>(); |
There was a problem hiding this comment.
See my other comment. This also applies to other instances in this file.
| jeff::Module::Builder module = message.initRoot<jeff::Module>(); | |
| jeff::Module::Builder mod = message.initRoot<jeff::Module>(); |
There was a problem hiding this comment.
Note to myself: Set up cpp-linter workflow afterward.
There was a problem hiding this comment.
Did you run the pre-commit hooks? The imports seem unsorted to me. 🤔
You can either set up prek or pre-commit and then run prek -a or pre-commit run -a, respectively.
| #define JEFF_QISKITC_VERSION_MAJOR @PROJECT_VERSION_MAJOR@ | ||
| #define JEFF_QISKITC_VERSION_MINOR @PROJECT_VERSION_MINOR@ | ||
| #define JEFF_QISKITC_VERSION_PATCH @PROJECT_VERSION_PATCH@ |
There was a problem hiding this comment.
Are these three used anywhere? 🤔
Summary
Adds
tools/jeff-qiskitc, a C++ tool that converts a jeffModuleinto aQiskit
QkCircuitvia the Qiskit C API. jeff -> Qiskit direction only, fornow. I'll add Qiskit -> jeff direction commit, after getting feedback from reviewers regarding
the design chosen here.
WellKnownGate/PauliProductRotationGate/QubitGate(gate_converter.*)map jeff's
QubitGate(wellKnown + controlled variants, and Pauli productrotations) onto the matching
QkGate/QkPauliProductRotation.Op/QubitOp/FloatOp/AllocOp/MeasureNdOp/GateOp(circuit_converter.*); each capnp union kind modeled as its own type
jeff_qiskitc.cppis the entry point: a resource-counting pass sizes theQkCircuitup front, then a second pass builds it.Currently covers
QubitOp'salloc/measureNd/gatevariants andOp'squbit/floatvariants.Testing
Added
tools/jeff-qiskitc/tests/gate_conversion_test.cpp, exercising everyentry in
WellKnownToQkGateMap/ControlledQkGateMapplus a handful ofPauli product rotations, wired up via a new
CMakeLists.txt(
ctest-registered). Locally: 216 assertions, 0 failures.Open question for maintainers
This repo has no existing C++ build/CI setup. The new
CMakeLists.txtneedsQISKIT_C_DIRpointed at aQiskit C API install (headers +
libqiskit.so), and I don't have visibilityinto how CI should obtain that; build from source, a prebuilt release
artifact, or something else? Would appreciate guidance before wiring this
into CI.
Test plan
cmake -S tools/jeff-qiskitc -B build -DQISKIT_C_DIR=<path>cmake --build buildctest --test-dir buildAI Disclosures
All the library code and the design decisions have been made by me. For the tests and cmake files, I used claude code.