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.
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
Add support for custom backend transpiler stages #8648
Add support for custom backend transpiler stages #8648
Changes from 11 commits
77d717f
061ebc4
d7c479d
5eae0ee
caa9c49
69e5b25
cf6a7ec
b845ac3
3870356
ccbbe55
e7b2e8b
f8e8fde
bb45d9d
d004080
214d9f7
4eb107b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there a more descriptive name that we can use here? I'm not sure that "backend default methods" will be clear to a user without context. Maybe something like
ignore_backend_defined_plugins
orignore_backend_supplied_plugins
?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 open to changing the name, what about
ignore_backend_supplied_default_methods
? I'm hesitant about the two suggestions because the plugin could be defined or supplied from somewhere else. For example, I could see a backend using thesynthesis
translation method (assuming it actually worked reliably which I'm skeptical of) which is supplied and defined in terra itself. Or more realistically a plugin defined in a different package that's not terra or the provider.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.
Also see my comment above trying to make the text a bit more clear....I didn't realize I had not done "finish your review".It's also not clear to me whether "method" means instance method or has the more generic, non-PL meaning. This is a recurrent problem in documentation.
EDIT: Re-reading the doc string, it's clear that this refers to an instance method.
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.
In the context of
transpile()
andgenerate_preset_pass_manager()
the "method" I'm referring to is the*method
kwargs (init_method
,layout_method
,routing_method
,translation_method
,optimization_method
,scheduling_method
). This new option is to disable changing the default values that aBackendV2
object (from thebackend
kwarg) can optionally set for these kwargs. I'm open to changing the word method if people have suggestions, but I was using it try and be consistent with the rest of thetranspile()
api.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.
Ok. I see that it's also clear in the doc that it means "instance method". It makes sense to me that whatever the name of the method in question here (eg
gnore_backend_supplied_default_methods
) , including the word "method" makes sense because it controls which methods will be called.(I don't know why my comment appears twice in my view of this page. Probably due to the way I finished the review.)
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 renamed it to
ignore_backend_supplied_default_methods
in d004080There 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 seems to me like restricting the flexibility of
StagedPassManager
where we can define transpiration with arbitrary combination of passes, i.e. the class can takestages
argument. I have no idea how current configuration of passes is generic, but alternatively backend can provide full set of staged passes rather than providing a particular subset passes that Qiskit transpiler defines. For example, some ecosystem software may offer a service that overrides an IBM backend with fancy pass manager that makes circuit more robust to noise -- I also think we no longer say this is a plugin. If this means to be a plugin, perhaps current implementation is fine.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 think there are 2 pieces here, for the preset passmanagers/
transpile()
vs a more generic interface. I think fortranspile()
we still want to have a more structured set of default stages. This was actually a big part of defining theStagedPassManager
for me was being able to formally define the stages for the default passmanagers to enable hooking into it like this. For this interface I was trying to enable backends to hook into the defaults stages with custom implementations (via plugins) that are specific for the needs of a backend. This is more about influencing the defaults to be better for the custom constraints of a particular backend. A user or a backend provider can still implement a fully customStagedPassManager
with additional or just different stages, but that is a lower level interface than what this PR is for. The idea for this is to basically enable users who runtranspile(circuit, backend)
where backend has custom constraints of some kind that the default preset passmanagers aren't able to handle. Having this interface enables the backend to easily keeptranspile()
working so those custom constraints can be handled cleanly without having to reimplement the entire pass manager and keep the rest of the user experience the same. For this PR I started with justtranslation
andscheduling
as I figured these stages were the most likely to be the ones which would have a need which was backend specific. The other thing is the default stages in the preset passmanager can (and probably will) be expanded in the future. So if we have a change in the global needs for the stages we use in the default pipeline we can add stages in the future (we've documented stability expectations around this in the class docs: https://qiskit.org/documentation/stubs/qiskit.transpiler.StagedPassManager.html?highlight=stagedpassmanager#qiskit.transpiler.StagedPassManager and basically said that additions are expected in the future).The second part is what the plugin usage. If you look at earlier iterations of this PR, I had this interface be a bit more generic similar to what you're describing here. Instead of working with plugins backends would return
PassManager
s for each stage implementation (see cf6a7ec where this changed). The consensus on this interface in discussions about this PR was that because we have a plugin interface for providing alternative implementations of the default stages (which was added in #8305) was that for this backend interface to leverage that new plugin interface would be ideal. This has 2 effects, it simplifies the code since we have a single entrypoint for adding custom methods for stages and also for custom stage implementations for a backend it advertises these over the standard plugin interface which hopefully makes them globally discoverable outside of just the default use when targeting a backend.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.
Okey, so we think current default stages of qiskit transpiler is sort of stable in future. My only concern was the stability of new API. To me current mechanism based off of #8305 seems reasonable.
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.
Should these be renamed to
get_translation_stage_plugin
?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.
Good catch, I'll update this now
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.
Actually I misread what you wrote here, I'm not sure we want to rename this. I had standardized on method because it's not actually restricted to plugins. It basically is the return of these methods are for changing the default for
translation_method
andscheduling_method
values ontranspile()
. They don't necessarily have to be a plugin (well at least until we make everything a plugin per #8661). But I can change the name if you'd prefer usingplugin
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.
Agree, I think this is clearer post- #8661 . I was trying to avoid the confusion between method as in
{layout,routing,scheduling}_method
and method as in "python function".I think either
get_translation_method
(matching the language fromtranspile(..., layout_method='foo')
orget_translation_stage_plugin
(matching the language from #8661 and #8305) would be okay.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 renamed it
get_$FOO_stage_plugin
in 214d9f7 (my shell messed up the commit message on it).