-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Implement a from_backend method for PassManagerConfig #7163
Conversation
It fetches configuration from a backend's configuration.
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.
Thanks for pushing this up, this is a really good idea for the class to simplify it's creation slightly. We can probably leverage this in the transpile()
function logic to simplify that web of code. While the PassManagerConfig
will be obsolete with the introduction of the the Target
class in #5885 it will still continue to exist for a long time until we can update everything to use the next backend interface version and then deprecate and remove it (which will probably be at least a year from the next release before we can deprecate it).
I left a few comments inline mostly on how this can be simplified. Most of the options in a PassManagerConfig
do not relate to the backend in use so we can just rely on __init__
to do most of the work here. Then for the 5 options that do potentially come from a backend we should handle them one by one because each has unique processing that needs to be done individually.
or the configuration does not support a `to_dict()` method. | ||
AttributeError: If the field passed in is not part of the options. | ||
""" | ||
res = PassManagerConfig() |
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 reason to do this instead of something like:
res = PassManagerConfig(**pass_manager_options)
and then you can change the rest of the logic to be something like:
config = backend.configuration()
if res.basis is None:
res.basis_gates = getattr(config, 'basis_gates', None)
for each of the backend attributes and then explicitly handle the construction for them.
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.
Thank you. I followed you suggestion, but with a slightly different style: res.basis_gates = res.basis_gates or getattr(config, "basis_gates", None)
. I think using or
makes the code a little more concise.
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.
The issue with using or
like this is that it will also treat empty containers as not being user specified which is why I used is None
in my example. For example, with the code like this if a user calls something like: PassManagerConfig.from_backend(FakeMelbourne(), basis_gates=[])
the output PassManagerConfig
object will have the basis gates of fake melbourne and not []
, while the user expectation would be that basis gates is []
because it was manually specified.
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.
You are right. Now I see the problem with or
. The code is updated.
@mtreinish Thank you so much for the detailed review. I learned a lot from it. As a newcomer, I am not familiar with the data structure used in Qiskit. Without your patient instructions, this task cannot be done. I am looking forward to continuing to participate in this project and learning from it! |
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.
The code looks much improved to me now, thanks for the fast updates. I think the only issue I see is the one I mentioned before where false evaluation for more than just the default None
which is probably undesirable behavior. I also suggested some change to the release notes which aren't critical.
releasenotes/notes/add-passmanager-config-from-backend-af5dd7d99ec053ef.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/add-passmanager-config-from-backend-af5dd7d99ec053ef.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/add-passmanager-config-from-backend-af5dd7d99ec053ef.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
…99ec053ef.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
…99ec053ef.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
…99ec053ef.yaml Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Thank you. I have updated the code. |
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 LGTM now, thanks for all the updates and sticking with it
Summary
The task is proposed by issue 7023. It proposes a
from_backend
method to ease the access of preset pass managers.Details and comments
Currently, the user needs to manually build a
PassManagerConfig
, which contains copying some backend options, e.g.This work can be alleviated if some method is provided. Issue #7023 proposes a method
from_backend(backend, **pass_manager_options)
that takes abackend
and user options as arguments, and returns aPassManagerConfig
object that can be ready to use. In this way, the above snippet can be simplified tolevel_1_pass_manager(from_backend(backend, **pass_manager_options))
. As to which class to add this method, Issue #7023 provides two options:PassManagerConfig
orFullPassManager
. Since the latter is still under review, this PR adds thefrom_backend
method toPassManagerConfig
. If later we find it is better to put it underFullPassManager
, the migration should be straightforward.This PR includes the following parts:
from_backend()
method (qiskit/transpiler/passmanager_config.py);