-
Notifications
You must be signed in to change notification settings - Fork 706
Implement ResourceConfig class #8195
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8195 +/- ##
==========================================
- Coverage 99.39% 99.39% -0.01%
==========================================
Files 550 550
Lines 57026 57026
==========================================
- Hits 56683 56682 -1
- Misses 343 344 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 have some architectural suggestions:
-
In general, we want to avoid having a global configuration object. In PL, a newExecutionConfigis created for each circuit execution. I think we want something similar here. Instead of having a global mutable object, the user would create anResourceConfigobject, modify it as they please, and manually pass this object to each execution of the resource estimation pipeline. -
The
ResourceConfigobject contains a lot of information, but most operators do not need any of that. The ones that do typically only need one or two values. I don't think thisResourceConfigshould be passed to each operator. The config object should store operator-specific configs on an operator-by-operator basis, using something along the lines of:{ ..., MPSPrep: {'precision': 1e-9}, Qubitization: {'rotation_precision': 15, 'coeff_precision': 15} }and the resource estimation pipeline is responsible for extracting the necessary keyword arguments for each operator, and passing them into the operator's decomp methods as regular keyword arguments. I'm thinking something along the lines of:
class SomeOp: @classmethod def resource_decomp(cls, ..., some_kwarg=...): ...
and then in the actual resource estimation pipeline,
kwargs = config.conf[cp_rep.op_type] resource_decomp = cp_rep.op_type.resource_decomp(**cp_rep.params, **kwargs)
This way we don't modify the signature of the resource decomp methods to include a
configthat most operators are not going to use, and the operators that do support customization don't have to dig the information they need out from a potentially very large dictionary with a a bunch of entries that are irrelevant to them.
Hi @astralcai , thanks for your suggestions and comments. I agree that the current design is not ideal: there is a lot of excess information that the config passes into the each resource_decomp method that is not necessary. The main issue is that we cannot avoid passing in the config into these resource_decomp methods. Here is an example that would fail without passing in config. Here, notice that if we do not pass in a config to our DummyOp's This leaves us with the following options:
|
|
Discussed offline. Conclusion is we will go with @astralcai's suggestions and rework the architecture essentially. The change in behaviour for |
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.
Looks good to me @austingmhuang
Co-authored-by: Diksha Dhawan <[email protected]>
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.
LGTM, thank you for addressing @astralcai 's concerns so quickly. ❤️
I think this PR has improved on the initial implementation a lot!
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 @austingmhuang . Please check the docs and also the arg names before merging.
Co-authored-by: Soran Jahangiri <[email protected]>
Co-authored-by: Soran Jahangiri <[email protected]>
Co-authored-by: Soran Jahangiri <[email protected]>
Co-authored-by: Soran Jahangiri <[email protected]>
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.
👁️
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.
✅
Context:
The resource config currently exists as a global dictionary, which is passed to plre.estimate
Issues:
Description of the Change:
Develops a config class, which has its own set of defaults, and makes it easy for the user to change what they wish.
Benefits:
Resolves above issues.
Possible Drawbacks:
Related GitHub Issues:
[sc-97227]