Expand backend specific default methods to cover all stages#16119
Expand backend specific default methods to cover all stages#16119mtreinish wants to merge 1 commit intoQiskit:mainfrom
Conversation
This commit expands the optional backend interface methods for specifying an alternative default plugin for a transpiler stage to cover all the stages in the preset pass manager. Originally this interface was introduced in Qiskit#8648 for the translation and scheduling stages. This was just a starting point as these were two stages most likely to require backend specific compilation. However, since then this interface has proven a useful tool for backend implementations but it's utility has been limited to just translation and scheduling. There is a potential role for a backend to provide an alternative default for the other stages of the preset pass manager. This finishes what Qiskit#8648 starts by expanding the interface to cover all the stages.
mtreinish
added
the
Changelog: Added
|
One or more of the following people are relevant to this code:
|
Coverage Report for CI Build 25186773179Coverage increased (+0.01%) to 87.583%Details
Uncovered Changes
Coverage Regressions11 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
| if not ignore_backend_supplied_default_methods: | ||
| if scheduling_method is None and hasattr(backend, "get_scheduling_stage_plugin"): | ||
| scheduling_method = backend.get_scheduling_stage_plugin() | ||
| if translation_method is None and hasattr(backend, "get_translation_stage_plugin"): | ||
| translation_method = backend.get_translation_stage_plugin() | ||
| if init_method is None and hasattr(backend, "get_init_stage_plugin"): | ||
| init_method = backend.get_init_stage_plugin() | ||
| if layout_method is None and hasattr(backend, "get_layout_stage_plugin"): | ||
| layout_method = backend.get_layout_stage_plugin() | ||
| if routing_method is None and hasattr(backend, "get_routing_stage_plugin"): | ||
| routing_method = backend.get_routing_stage_plugin() | ||
| if optimization_method is None and hasattr(backend, "get_optimization_stage_plugin"): | ||
| optimization_method = backend.get_optimization_stage_plugin() |
There was a problem hiding this comment.
Given we unconditionally call the method if it exists and expect a certain behaviour, I think this is functionally no different to adding
class Backend:
def get_optimization_stage_plugin(self) -> None | str:
return None
# ...with all the not-entirely-stable implications that comes with. Because this is called unconditionally and transpile is typed as taking Backend, I want to point out this has some implications for BackendV3 too as transpile is currently written. The documentation is only in BackendV2, but the only actual behaviour of transpile has been to do it for BackendV1 as well.
This was of course true in #8648 too and it's somewhat regrettable we didn't talk about it then (also wow, it's fairly clear how our attitudes to stability where different back then).
Anyway, my point:
- the potential for breakage here is certainly unlikely, but technically entirely possible, so I think at a minimum we need an
upgradenote - can we / should make these proper methods on the
Backendbase class? I think we should at least consider this forBackendV1andBackendV2objects, and also fix the type hinting oftranspileto be in terms only of those two concrete types - I think we should really not have this implied behaviour forBackendV3too (and I don't think it's guaranteed the same hardcoded stage names will make sense there).
There was a problem hiding this comment.
Well BackendV1 we removed in 2.0 to get rid of the dead weight model objects, so there is no concern there anymore. I was thinking for backendv3 we'd keep this same interface because I do think having a way for a backend to define their own compilation routines by default is important. I agree it's not the prettiest interface but we're kind of stuck with it now for backendv2 at least. We have the opportunity to improve on this for V3 if you have suggestions on how we should handle backend specific custom compilation. It's very easy to update the logic in this PR to wrap everything in a if backend.version == 2 to make it more future facing.
I agree there is some concern around backwards compatibility but it really depends on how we want to handle our stability contract around out of tree backend objects. Like on it's own this won't change anything unless a backend opts-in to using the new methods. That being said I can add an upgrade note about it to just warn users that it might change compilation for backends in the future and they should refer to the backend providers for changes.
As for proper methods yeah we can do that, I just was mirroring the existing pattern we had from #8648 but having explicit methods would probably be better just from a documentation PoV if nothing else.
There was a problem hiding this comment.
In general I'm ok expanding the interface to include the extra stages here, it's mostly the documentation I'm concerned with.
The reason I say it's unstable is that an existing BackendV2 could technically have defined a get_optimization_stage_plugin method of its own with an arbitrary signature, and until this PR, it would have been fine. Following this PR, it could suddenly be invalid to pass that object to transpile.
I think we definitely should type-check on ifinstance(BackendV2) before doing these checks, since they're only documented as being valid on BackendV2, which would solve some of my concern. I think we'd need to have a clearer view on the compilation pipeline for QuantumProgram/BackendV3 before we start making decisions about that.
The (not especially strong) concern about the instability of assigning new meaning to existing get_optimization_stage_plugin methods isn't solvable by an isinstance check; prior to Qiskit 2.5, it was valid to have a BackendV2 with such a method and a totally different signature / semantics. If we were going to do that, we'd need some sort of separate versioning semantics on the subclass interface, which don't exist - something like a backend_feature_level() hook that returns a version of the BackendV2 spec, and we only attempt to access the new-in-Qiskit-2.5 methods for backends that set that feature level high enough. Given we'd have to break semver compatibility in the same way to patch that method/hook onto BackendV2, I don't think it's worth doing, but I think that kind of version negotiation is probably what we want in the future if we're intending to define forwards-compatible subclass interfaces.
There was a problem hiding this comment.
Oh you meant on that front, I agree it is a breaking change since someone could have been using the name before. I just assumed in practice it is extremely unlikely especially given the existing get_translation_stage_plugin and get_scheduling_stage_plugin methods and we could slip it in without any complaints. One option is I can put protections in place to catch any exceptions caused by trying to call the methods, and also gracefully handling the case where the method exists already and has the same signature but doesn't return a valid plugin name. The only case we can't handle is if a backend was previously using this method name with the same signature and the returned string is a valid plugin name, but the intent of the pre-existing method was not to return a default plugin name. It did seem a very unlikely scenario in practice.
The other two options are we can just save this for BackendV3 and not bother with it on BackendV2, which seems reasonable to me. The other option is wait and indeterminately long period of time until Qiskit 3.0 for adding it on BackendV2. Actually talking this through I'm thinking doing both is sufficient. I didn't have a strong use case in mind for this. I was just musing with the idea that exposing this interface would be a nice avenue to investigate adding backend specific optimizations, like sabre prelayout hints and similar in the future.
| * ``get_init_stage_plugin`` | ||
| * ``get_layout_stage_plugin`` | ||
| * ``get_routing_stage_plugin`` | ||
| * ``get_translation_stage_plugin`` | ||
| * ``get_scheduling_stage_plugin``. |
| The typical expected use case is for a backend provider to implement a stage | ||
| plugin that contains the custom compilation passes and then for the hook methods on | ||
| the backend object to return the plugin name so that :func:`~.transpile` will | ||
| use it by default when targeting the backend. |
There was a problem hiding this comment.
This isn't "the typical expected use case", I don't think? It's a description of the actual calling signature. It might be clearer by writing out the actual signature (although see the other review comment about just making them overrideable methods).
| runnable on the backend. These hooks are enabled by default and should only | ||
| be used to enable extra compilation steps if they are **required** to ensure | ||
| a circuit is executable on the backend or have the expected level of performance. | ||
| These methods are passed no input arguments and are expected to return a ``str`` |
There was a problem hiding this comment.
This new text adds in the bit about "bespoke methods" in as an additional the "custom requirements" reasons, but that then sounds weird
with the later sentence
These hooks are enabled by default and should only be used to enable extra compilation steps if they are required to ensure a circuit is executable on the backend or have the expected level of performance.
- they're "enabled by default" but we don't have a comment on how to disable them (it's a user setting of
transpile, not aBackendV2-author setting) - I think the whole rest of the sentence about "should only be used ..." is kind of unnecessary? It boils down to "if you believe Qiskit's defaults are not optimal for your backend", which might be a cleaner way of phrasing more of this text (since the context of the text has changed a little now it's every stage, and not just translation/scheduling).
| A backend object can optionally contain methods named: | ||
| * ``get_init_stage_plugin`` | ||
| * ``get_layout_stage_plugin`` | ||
| * ``get_routing_stage_plugin`` | ||
| * ``get_translation_stage_plugin`` | ||
| * ``get_scheduling_stage_plugin``. |
There was a problem hiding this comment.
Also the docs-build error is a true error and it's complaining that these bullet points are indented by a space, when they shouldn't be indented at all (but you might need a blank line before and after them?).
|
Oh yeah, I forgot to write in the top review comment: given we already did this once in #8648, I think I'm roughly on board with doing it again now, even though I don't think it's technically semver stable to add extra methods to a base class in a minor release. I would like to see if we can design |
The simplest thing we could probably do is for any defined transpiler stage plugin interface have the name |
|
If we're going to do future compatibility by reserved names, I think we need to reserve a way larger namespace of methods, like |
4 participants
This commit expands the optional backend interface methods for specifying an alternative default plugin for a transpiler stage to cover all the stages in the preset pass manager. Originally this interface was introduced in #8648 for the translation and scheduling stages. This was just a starting point as these were two stages most likely to require backend specific compilation. However, since then this interface has proven a useful tool for backend implementations but it's utility has been limited to just translation and scheduling. There is a potential role for a backend to provide an alternative default for the other stages of the preset pass manager. This finishes what #8648 starts by expanding the interface to cover all the stages.
AI/LLM disclosure