-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add qk_transpile_stage_routing() to the C API #15358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
One or more of the following people are relevant to this code:
|
| None | ||
| } else { | ||
| if !(0.0..=1.0).contains(&options.approximation_degree) { | ||
| panic!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi thank you so much for your contribution, this is a great PR with perfect modularization of the stages in :meth:transpile function for the C API, I had some quick clarifications however.
In this line, in specific, I am confused about the usage of a panic. I inspected the :class:TranspileOptions, from which, I could see that it accepts any f64 value, unconstrained by range. So my question is, why do we panic here, on incorrect user input, while we could Exit here with an error CString instead as is done later in the control flow. I understand we don't have normalized errors yet and hence we resort to panics and PyErrs, but is the CString based exit not safer than the complete process abortion from panic? I would love to know what your thoughts on this were while working on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing an value outside 0 and 1 will error inside the transpiler rust code because those are the only allowed values. The container TranspileOptions doesn't have a mechanism to enforce this because it's a struct exposed to C. So even if we had a rust space constructor that did input validation the user could still do:
TranspileOptions transpile_options = qk_default_transpile_options();
transpile_options.approximation_degree = 3.14159and that wouldn't error. This is why we need to do the checking before we pass the options to the inner stage function so we don't fail at a random spot internally. As for why this is a panic, this API isn't really interactive. We document the only values are between 0 and 1 are valid and if the user violates that it's not really something we should recover from because it's a programming error. If a user called (in a simplified form):
qk_transpile_stage_routing(.., approximation_degree=3.14159)There isn't really something for the user to recover it's an invalid call.
| /// respectively. ``options`` must be a valid pointer a to a ``QkTranspileOptions`` or ``NULL`. | ||
| /// ``error`` must be a valid pointer to a ``char`` pointer or ``NULL``. | ||
| #[unsafe(no_mangle)] | ||
| #[cfg(feature = "cbinding")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small cleanup suggestion I could think of here is that several of the new :meth:qk_transpile_stage_* functions repeat the same pointer-conversion, options-handling, approximation-degree parsing, seed parsing, and error-reporting boilerplate. It may be worth considering internal helpers to centralize that logic and reduce duplication. This changed is not required for this PR however, as the overall functionality doesn't change. But the code reduction may improve maintainability and readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fair comment, but I think since this PR is adding the last one in the series we should just finish following the pattern the other functions use. But if you want to push a PR on top of this one deduplicating the internals we can review that very easily if it makes things cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you so much for your contibution, this PR is well written and LGTM, there is however just a couple of comments I had, for my understanding. Thank you
51086ae to
99da6a9
Compare
This commit adds a new function to the C API, `qk_transpile_stage_routing()`, which is used to run just the routing stage of the preset pass manager. The intended use case for this is to enable using stages from the preset pass manager with custom transpiler passes to build custom compilation workflows from C.
99da6a9 to
bb54235
Compare
|
Now that #15295 has merged I've rebased this on main and it is unblocked from review/merging now. |
Pull Request Test Coverage Report for Build 19974534105Details
💛 - Coveralls |
raynelfss
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to report that you finally got documentation in the first try. Though I do have some formatting suggestions regarding the release note.
| * :c:func:`qk_transpile_stage_init` | ||
| * :c:func:`qk_transpile_stage_layout` | ||
| * :c:func:`qk_transpile_stage_routing` | ||
| * :c:func:`qk_transpile_stage_translation` | ||
| * :c:func:`qk_transpile_stage_optimization` | ||
| run the :ref:`transpiler-preset-stage-init`, :ref:`transpiler-preset-stage-layout`, | ||
| :ref:`transpiler-preset-stage-routing`, | ||
| :ref:`transpiler-preset-stage-translation`, and | ||
| :ref:`transpiler-preset-stage-optimization` respectively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to turn these into a table? Sort of like this:
| * :c:func:`qk_transpile_stage_init` | |
| * :c:func:`qk_transpile_stage_layout` | |
| * :c:func:`qk_transpile_stage_routing` | |
| * :c:func:`qk_transpile_stage_translation` | |
| * :c:func:`qk_transpile_stage_optimization` | |
| run the :ref:`transpiler-preset-stage-init`, :ref:`transpiler-preset-stage-layout`, | |
| :ref:`transpiler-preset-stage-routing`, | |
| :ref:`transpiler-preset-stage-translation`, and | |
| :ref:`transpiler-preset-stage-optimization` respectively. | |
| ========================================= =========================================== | |
| C function Transpiler Stage | |
| ========================================= =========================================== | |
| :c:func:`qk_transpile_stage_init` :ref:`transpiler-preset-stage-init` | |
| :c:func:`qk_transpile_stage_layout` :ref:`transpiler-preset-stage-layout` | |
| :c:func:`qk_transpile_stage_routing` :ref:`transpiler-preset-stage-routing` | |
| :c:func:`qk_transpile_stage_translation` :ref:`transpiler-preset-stage-translation` | |
| :c:func:`qk_transpile_stage_optimization` :ref:`transpiler-preset-stage-optimization` | |
| ========================================= =========================================== | |
It would look like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way. I went with a list because it was simpler. Either way we can always change it in the release note roundup for the final release.
Summary
This commit adds a new function to the C API,
qk_transpile_stage_routing(), which is used to run just the routing stageof the preset pass manager. The intended use case for this is to enable
using stages from the preset pass manager with custom transpiler passes
to build custom compilation workflows from C.
Details and comments
This PR is based on top of #15295 and will need to be rebased once that merges. To view the contents of just this PR you can look at the HEAD commit on this PR branch: 51086aeThis has been rebased on main.