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: Constant values in hugr-model #1838

Merged
merged 8 commits into from
Jan 7, 2025
Merged

feat: Constant values in hugr-model #1838

merged 8 commits into from
Jan 7, 2025

Conversation

zrho
Copy link
Contributor

@zrho zrho commented Jan 6, 2025

Import and export constant values via hugr-model.

Includes terms corresponding to Value::Function and allows exporting; importing these is left for a later PR since sum/extension constants are the most important for now. In the future, we should move from the JSON/typetag based model for extension constants to one based on custom constructors.

@hugrbot
Copy link
Collaborator

hugrbot commented Jan 6, 2025

This PR contains breaking changes to the public Rust API.
Please deprecate the old API instead (if possible), or mark the PR with a ! to indicate a breaking change.

cargo-semver-checks summary

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_variant_added.ron

Failed in:
variant Term:Const in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:571
variant Term:ConstFunc in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:677
variant Term:ConstAdt in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:683
variant Operation:Const in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-model/src/v0/mod.rs:380

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_variant_missing.ron

Failed in:
variant Term::Quote, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-model/src/v0/mod.rs:565

@zrho zrho force-pushed the zrho/model-const-func branch 2 times, most recently from 2bc0a18 to 565e18a Compare January 6, 2025 13:08
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 65.97938% with 99 lines in your changes missing coverage. Please review.

Project coverage is 86.55%. Comparing base (09cbc6a) to head (030e601).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
hugr-core/src/import.rs 54.25% 17 Missing and 26 partials ⚠️
hugr-core/src/export.rs 85.71% 15 Missing and 2 partials ⚠️
hugr-model/src/v0/text/print.rs 47.61% 8 Missing and 3 partials ⚠️
hugr-model/src/v0/binary/read.rs 41.17% 9 Missing and 1 partial ⚠️
hugr-model/src/v0/text/parse.rs 52.38% 4 Missing and 6 partials ⚠️
hugr-model/src/v0/binary/write.rs 42.85% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1838      +/-   ##
==========================================
- Coverage   86.67%   86.55%   -0.13%     
==========================================
  Files         194      194              
  Lines       35010    35156     +146     
  Branches    31823    31969     +146     
==========================================
+ Hits        30346    30430      +84     
- Misses       2939     2968      +29     
- Partials     1725     1758      +33     
Flag Coverage Δ
python 92.31% <ø> (ø)
rust 85.98% <65.97%> (-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.

@zrho zrho marked this pull request as ready for review January 6, 2025 13:16
@zrho zrho requested a review from a team as a code owner January 6, 2025 13:16
@zrho zrho requested a review from croyzor January 6, 2025 13:16
@ss2165 ss2165 requested review from ss2165 and removed request for croyzor January 7, 2025 10:12
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

Looks good, biggest question is about -> placement

flamegraph.svg Outdated Show resolved Hide resolved
Comment on lines +75 to +78
node_to_id: FxHashMap<Node, model::NodeId>,

/// Mapping from node ids in the [`Hugr`] to the corresponding model nodes.
id_to_node: FxHashMap<model::NodeId, Node>,
Copy link
Member

Choose a reason for hiding this comment

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

if these are meant to stay in sync would be better to have a dedicated struct holding both that has responsibility for their correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally yes. I'm not entirely sure yet if the way that functions are exported is ideal here, so there might be a bit of churn around these things happening. In that light I'm not sure how much it is worth to polish.

Copy link
Member

Choose a reason for hiding this comment

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

leave a todo perhaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO added.

hugr-core/src/export.rs Show resolved Hide resolved
hugr-core/src/export.rs Show resolved Hide resolved
hugr-core/src/import.rs Show resolved Hide resolved
hugr-core/src/import.rs Show resolved Hide resolved
hugr-core/src/import.rs Show resolved Hide resolved
hugr-model/src/v0/binary/read.rs Show resolved Hide resolved
hugr-model/src/v0/text/print.rs Show resolved Hide resolved
hugr-core/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

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

LGTM modulo CI fixes 👍

@zrho zrho force-pushed the zrho/model-const-func branch from 4cc2572 to 3a50604 Compare January 7, 2025 14:40
@zrho zrho added this pull request to the merge queue Jan 7, 2025
Merged via the queue into main with commit b36d97d Jan 7, 2025
22 of 24 checks passed
@zrho zrho deleted the zrho/model-const-func branch January 7, 2025 15:20
@hugrbot hugrbot mentioned this pull request Jan 7, 2025
@hugrbot hugrbot mentioned this pull request Jan 16, 2025
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.

3 participants