-
Notifications
You must be signed in to change notification settings - Fork 80
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
Parse manifest in Rust instead of Python #1330
Conversation
Benchmark for 9f91728Click to view benchmark
|
Benchmark for 9cebb9aClick to view benchmark
|
Benchmark for c481cc1Click to view benchmark
|
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 afraid this change will interfere with how the notebook language service works (for which I'm realizing there's no integration tests, so nothing in the CI broke).
# Return the configuration information to provide a hint to the
# language service through the cell output.
return Config(target_profile, language_features)
This line is critical for the language service - it causes the qsharp.json
configuration to be written out to the notebook cell output, which the language service will read to configure compiler options. (Can give you a demo to show you what I mean, but you can try by changing languageFeatures
and re-running qsharp.init
to see how the squiggles change in the editor).
I think this requirement forces us to parse the qsharp.json at the Python level. Even if we parse it in Rust, we'd need to return the contents of the manifest back to Python... which will be too convoluted to be worth it IMO.
I implemented the solution we talked about. Passing the raw manifest around and parsing it when it gets to rust land. |
Benchmark for b1d25bfClick to view benchmark
|
Benchmark for 0386608Click to view benchmark
|
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.
Looking good but there seems to be a type error when sending the manifest through the language service.
I think this is the kind of scenario that would benefit from an integration test. LMK if you need help running/authoring those.
Co-authored-by: Mine Starks <16928427+minestarks@users.noreply.github.com>
Benchmark for 6a1889fClick to view benchmark
|
Benchmark for f7dfd63Click to view benchmark
|
Benchmark for e500567Click to view benchmark
|
I fixed the type mismatch. I will add the integration test to #1313 instead of this PR. This is one is ready for review. |
Benchmark for 43ac472Click to view benchmark
|
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. Thanks!
This PR regains some rust real-state. It delegates the parsing of the manifest in
pip/qsharp/_qsharp.py/init()
to rust, avoiding duplicate manifest-parsing logic in the project.This change is needed to unblock #1313