-
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: Packages in hugr-model
and envelope support.
#2026
base: main
Are you sure you want to change the base?
Conversation
882d225
to
7822310
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2026 +/- ##
==========================================
- Coverage 83.09% 82.96% -0.14%
==========================================
Files 215 215
Lines 40905 41075 +170
Branches 37119 37273 +154
==========================================
+ Hits 33992 34077 +85
- Misses 5027 5107 +80
- Partials 1886 1891 +5
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:
|
d011326
to
7e48e3a
Compare
65710d8
to
6d7776b
Compare
6d7776b
to
2e69e9a
Compare
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.
Cool. I mostly have a question about APIs
hugr-core/src/export.rs
Outdated
@@ -28,7 +28,7 @@ use petgraph::unionfind::UnionFind; | |||
use std::fmt::Write; | |||
|
|||
/// Export a [`Hugr`] graph to its representation in the model. | |||
pub fn export_hugr<'a>(hugr: &'a Hugr, bump: &'a Bump) -> table::Module<'a> { | |||
pub(crate) fn export_hugr<'a>(hugr: &'a Hugr, bump: &'a Bump) -> table::Module<'a> { |
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.
Check the semver-checks bot warnings!
If we don't want to expose this publicly anymore, it should be deprecated
first (and leave a TODO indicating what to do in the next breaking release).
Edit: See my comment on hugr.rs
about whether we should leave these un-deprecated.
@@ -34,6 +34,8 @@ | |||
//! - Bit 7,6: Constant "01" to make some headers ascii-printable. | |||
//! | |||
|
|||
#![allow(deprecated)] |
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 see you got bitten by JelteF/derive_more#419 -.-
Can you add a TODO to remove this once derive_more
fixes that?
(if only we had expect
...)
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've added the TODO comment with the links.
hugr-core/src/hugr.rs
Outdated
pub fn to_model<'a>(&'a self, bump: &'a model::bumpalo::Bump) -> model::table::Module<'a> { | ||
export_hugr(self, bump) | ||
} | ||
|
||
/// Import a module from the model representation. | ||
#[cfg(feature = "model_unstable")] | ||
pub fn from_model<'a>( |
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 think we want this as a first-class HUGR method, as user should normally go via packages instead (esp. once #2029 is in.
I'd leave it in hugr::{im,ex}port::{im,ex}port_hugr
instead. We'll add a to_envelope
or something here once thing get implemented.
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 also prefer to have this as the import_*
and export_*
functions. I had switched to these methods for consistency with the existing design, but it does make sense that ultimately we're going to use envelopes anyway. So it's reverted to the functions; that should address multiple comments.
hugr-core/src/package.rs
Outdated
|
||
/// Export this package to the model representation. | ||
#[cfg(feature = "model_unstable")] | ||
pub fn to_model<'a>(&'a self, bump: &'a model::bumpalo::Bump) -> model::table::Package<'a> { |
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.
Same here, do we want user to ever call this instead of load
/store
?
hugr-py/src/hugr/model/__init__.py
Outdated
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.
Package
methods missing docstrings
hugr-py/src/hugr/envelope.py
Outdated
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.
Can we have some tests for module envelopes?
(see test_envelope.py)
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.
We can't roundtrip on the Python side yet since the equivalent of import
does not exist yet, since that is less relevant currently than being able to write. I agree that there should be more tests; in the interest of getting this in a release asap and fixing it later (which appears to be the strategy we're going for) this PR might not be the place.
81b4fb8
to
a0cb97c
Compare
This PR adds packages to hugr-model with support for envelopes.