-
Notifications
You must be signed in to change notification settings - Fork 5
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
Export rust QCS ClientConfiguration to python #235
Conversation
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.
According to maturin we want a <module_name>.pyi
in the root for each module, but it isn't explicit about what to do for submodules. I would try just the module name first to see if that works. If not, maybe try the full module path? Ie. qcs_sdk.my_module.pyi
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 don't quite follow this sentence:
The info method helps with testing, but one could mistake the output for being serializable when it isn't really. Any thoughts on if we should provide a serialized view of the client, or what should be inspectable by the client?
Has that already been removed, or am I missing it in review? I think we're okay without serializing the client; it's just a handle on file data anyway (which is a neat serialized format) and there are getters and setters for everything, so I can't think of anything the user would need to do but couldn't.
Overall, this looks good to me, and neat 🗃️ . I'd like to ask for a look from @Shadow53 at the macro invocation for a full approval.
There's also the pyi type hints - I don't have any insight there and would likely just try the same things you are. If nothing else, make sure to clean up any experiments/cruft prior to final review & merge.
Other than that - I'd be happy to approve 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.
Minor comment on the type hints, but looks good!
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.
🚀
* chore: add helper for listing qpu names * chore: mr feedback * chore: paginate with timeout * chore: comment update * Update crates/python/src/api.rs Co-authored-by: Michael Bryant <mbryant@rigetti.com> * chore: use tokio timeout * chore: better error verbiage * chore: fix imports add test * chore: use namespacing instead of renaming * chore: use error converter helper * chore: remove superfluous gitignore Co-authored-by: Michael Bryant <mbryant@rigetti.com>
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 Rust looks good, except for a couple clarifications. There are things to be corrected with the pyi
files, though.
I started to comment each instance, but I was basically repeating myself for the same 3-4 things, so I instead left a comment pointing out how a thing should be fixed and then left it as an exercise for the reader to find the others.
I'm open to pairing on this if there are any questions about how the pyi files should be written.
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.
Left a few notes. The big one is that there are types in the pyi files with @property
getters but not setters, but I think they get setters from the macros
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.
A couple more places that might need setters in pyi
, and a question whether a type can be a pyclass
instead of a wrapped type.
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
Closes #231
Openquestions:Theinfo
method helps with testing, but one could mistake the output for being serializable when it isn't really. Any thoughts on if we should provide a serialized view of the client, or what should be inspectable by the client?Implemented the
==
operator overload without exposingDebug
output.How do we addUse https://www.maturin.rs/project_layout.html#mixed-rustpython-project but only havepyi
type stubs for nested submodules? Should everything be exported at the top level?.pyi
files.