Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

zrho
Copy link
Contributor

@zrho zrho commented Mar 25, 2025

This PR adds packages to hugr-model with support for envelopes.

  • Packages for both the AST and table representation of hugr-model.
  • Packages in the Python model classes.
  • Reading and writing model envelopes in Rust.
  • Writing model envelopes in Python.

@zrho zrho force-pushed the zrho/model-package branch from 882d225 to 7822310 Compare March 26, 2025 10:13
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 52.73632% with 95 lines in your changes missing coverage. Please review.

Project coverage is 82.96%. Comparing base (4734948) to head (f1789a9).

Files with missing lines Patch % Lines
hugr-model/src/capnp/hugr_v0_capnp.rs 24.39% 61 Missing and 1 partial ⚠️
hugr-model/src/v0/ast/python.rs 0.00% 11 Missing ⚠️
hugr-py/src/hugr/envelope.py 30.00% 7 Missing ⚠️
hugr-py/src/hugr/model/__init__.py 66.66% 4 Missing ⚠️
hugr-core/src/envelope.rs 25.00% 0 Missing and 3 partials ⚠️
hugr-model/src/v0/binary/read.rs 80.00% 0 Missing and 3 partials ⚠️
hugr-core/src/import.rs 91.66% 0 Missing and 1 partial ⚠️
hugr-model/src/v0/ast/mod.rs 90.90% 0 Missing and 1 partial ⚠️
hugr-model/src/v0/ast/parse.rs 87.50% 0 Missing and 1 partial ⚠️
hugr-model/src/v0/table/mod.rs 87.50% 0 Missing and 1 partial ⚠️
... and 1 more
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     
Flag Coverage Δ
python 85.79% <52.00%> (-0.13%) ⬇️
rust 82.67% <52.84%> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zrho zrho marked this pull request as ready for review March 26, 2025 12:09
@zrho zrho requested a review from a team as a code owner March 26, 2025 12:09
@zrho zrho requested review from tatiana-s and ss2165 March 26, 2025 12:09
@zrho zrho force-pushed the zrho/model-package branch from d011326 to 7e48e3a Compare March 26, 2025 13:02
@ss2165 ss2165 requested review from aborgna-q and removed request for ss2165 March 26, 2025 16:16
@zrho zrho force-pushed the zrho/model-package branch from 65710d8 to 6d7776b Compare March 26, 2025 17:02
@zrho zrho force-pushed the zrho/model-package branch from 6d7776b to 2e69e9a Compare March 26, 2025 17:37
Copy link
Collaborator

@aborgna-q aborgna-q left a 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

@@ -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> {
Copy link
Collaborator

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)]
Copy link
Collaborator

@aborgna-q aborgna-q Mar 31, 2025

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...)

Copy link
Contributor Author

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.

Comment on lines 269 to 275
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>(
Copy link
Collaborator

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.

Copy link
Contributor Author

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.


/// 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> {
Copy link
Collaborator

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package methods missing docstrings

Copy link
Collaborator

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)

Copy link
Contributor Author

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.

@zrho zrho force-pushed the zrho/model-package branch from 81b4fb8 to a0cb97c Compare March 31, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants