-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Python bindings for hugr-model
.
#1959
base: main
Are you sure you want to change the base?
Conversation
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1959 +/- ##
==========================================
- Coverage 83.73% 82.55% -1.19%
==========================================
Files 209 212 +3
Lines 39266 40061 +795
Branches 35937 36265 +328
==========================================
+ Hits 32879 33071 +192
- Misses 4541 5144 +603
Partials 1846 1846
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
49fd266
to
f7c4ba4
Compare
related #882 |
@@ -4,7 +4,7 @@ use std::str::FromStr; | |||
|
|||
use hugr::std_extensions::std_reg; | |||
use hugr_core::{export::export_hugr, import::import_hugr}; | |||
use hugr_model::v0 as model; | |||
use hugr_model::v0::{self as model}; |
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.
Unneeded diff?
use hugr_model::v0::{self as model}; | |
use hugr_model::v0 as model; |
@@ -29,6 +29,7 @@ pest_derive = { workspace = true } | |||
pretty = { workspace = true } | |||
smol_str = { workspace = true, features = ["serde"] } | |||
thiserror.workspace = true | |||
pyo3 = { workspace = true, optional = true } |
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.
nitpick: Make the feature list explicit (so it's easier to read)
pyo3 = { workspace = true, optional = true } | |
pyo3 = { workspace = true, optional = true } | |
[features] | |
pyo3 = ["dep:pyo3"] |
[package] | ||
name = "hugr-py" | ||
version = "0.1.0" | ||
edition = "2021" | ||
|
||
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html |
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.
Adding publish = false
here, to ensure this never gets released as a crate.
[package] | |
name = "hugr-py" | |
version = "0.1.0" | |
edition = "2021" | |
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | |
[package] | |
name = "hugr-py" | |
version = "0.0.0" | |
edition = { workspace = true } | |
rust-version = { workspace = true } | |
publish = false | |
[lints] | |
workspace = true | |
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.
Or this may also be a good place to start using edition2024 (and msrv 1.85) :)
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.
drive-by: Can you remove the upper bound from the requires-python
here?
[tool.maturin] | ||
features = ["pyo3/extension-module"] | ||
python-source = "src/" | ||
module-name = "hugr._hugr" |
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.
Pyright lint config imported from tket2
module-name = "hugr._hugr" | |
module-name = "hugr._hugr" | |
[tool.pyright] | |
# Rust bindings have typing stubs but no python source code. | |
reportMissingModuleSource = "none" | |
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.
Remove 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.
Missing docstrings
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.
Missing docstrings
@@ -82,6 +91,10 @@ def resolve(self, registry: ext.ExtensionRegistry) -> Type: | |||
"""Resolve types in the type using the given registry.""" | |||
return self | |||
|
|||
def to_model(self) -> model.Term | model.Splice: | |||
"""Convert the type to a model Term.""" | |||
raise NotImplementedError(self) |
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 absence of implementations for Type
, TypeParam
, TypeArg
seem like significant omissions! Is there a reasoning behind it?
@@ -103,6 +116,10 @@ def _to_serial(self) -> stys.TypeTypeParam: | |||
def __str__(self) -> str: | |||
return str(self.bound) | |||
|
|||
def to_model(self) -> model.Term: | |||
# Note that we drop the bound. |
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.
Why are we dropping the bounds here and in BoundedNatParam
?
@@ -656,6 +759,10 @@ def _to_serial(self) -> stys.Qubit: | |||
def __repr__(self) -> str: | |||
return "Qubit" | |||
|
|||
def to_model(self) -> model.Term: | |||
# TODO: Is this the correct name? | |||
return model.Apply("prelude.Qubit", []) |
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.
In hugr-core it's lowercase "qubit"
This PR introduces Python bindings to
hugr-model
.hugr-py
data structures into the Python AST.Until spec ambiguities about uniqueness of names are decided, the export procedure mangles the names of symbols by adding the id of the defining node.