feat: Draft for `hugr-model` with export, import, parsing and pretty printing #1542

zrho · 2024-10-02T13:32:58Z

This PR defines a draft of the model format, and the following parts that operate on the format:

Import: A module in the model format can be imported as a core hugr module.
Export: A core hugr module can be exported into the model format.
Parsing: A model hugr can be parsed from an s-expression based text format.
Pretty printing:: A model hugr can be printed into the s-expression format, with readable indentation.

This PR is very big, and yet it does not present a completed feature. The model and core format differ in several aspects. Where they differ, this is mostly intentional: The model is kept flexible enough to incorporate features that we know that we want in the future but are currently hard or impossible to express in the core. Import and export perform conversions where possible, but do not yet cover everything.

To prevent this PR from becoming even bigger and to allow for core and model to converge incrementally from both directions, I suggest the following plan: We merge this PR into main with the model related code being feature gated and considered experimental. We then perform tweaks to model and core in smaller PRs where necessary until they are sufficiently close.

codecov · 2024-10-02T13:35:23Z

Codecov Report

Attention: Patch coverage is 69.09570% with 704 lines in your changes missing coverage. Please review.

Project coverage is 85.71%. Comparing base (ea8e818) to head (706b7ac).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/import.rs	70.15%	122 Missing and 109 partials ⚠️
hugr-model/src/v0/text/print.rs	59.32%	116 Missing and 76 partials ⚠️
hugr-model/src/v0/text/parse.rs	65.89%	70 Missing and 92 partials ⚠️
hugr-core/src/export.rs	82.55%	67 Missing and 19 partials ⚠️
hugr-model/src/v0/mod.rs	42.10%	33 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1542      +/-   ##
==========================================
- Coverage   87.43%   85.71%   -1.72%     
==========================================
  Files         127      132       +5     
  Lines       21740    24017    +2277     
  Branches    18740    21017    +2277     
==========================================
+ Hits        19008    20587    +1579     
- Misses       1964     2366     +402     
- Partials      768     1064     +296

Flag	Coverage Δ
python	`92.46% <ø> (ø)`
rust	`84.75% <69.09%> (-1.88%)`	⬇️

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.

hugr-model/src/v0/mod.rs

ss2165

first review of mod.rs

hugr-model/src/v0/mod.rs

hugr-core/tests/model.rs

hugr-model/src/v0/mod.rs

aborgna-q

Some general comments, I'll do mod/export next.

Other tidbits:

Add hugr-model to .github/change-filters.yml

Cargo.toml

hugr-core/src/lib.rs

hugr-model/Cargo.toml

hugr-model/README.md

hugr-model/src/v0/mod.rs

Co-authored-by: Agustín Borgna <[email protected]>

Also added hugr model to change-filters.yaml

cqc-alec

(Looked at parse.rs and print.rs.) LGTM, I just highlighted some TODO comments that you may or may not want to address here (if not we should make issues).

hugr-model/src/v0/text/print.rs

hugr-model/src/v0/text/parse.rs

ss2165

import first pass, second half coming soon

hugr-core/src/import.rs

ss2165

import round 2

hugr-core/src/import.rs

ss2165 · 2024-10-04T12:54:28Z

hugr-core/src/import.rs

+                Err(error_uninferred!("application with implicit parameters"))
+            }
+
+            model::Term::ApplyFull { name, args } => {


while I understand the rationale I find the encoding of runtime custom types as "applyfull" still confusing on first interaction. Either a rename or some comments here would help.

I've added some docs by now. Does it become clearer?

hugr-core/src/import.rs

Co-authored-by: Seyon Sivarajah <[email protected]>

aborgna-q · 2024-10-04T13:51:09Z

hugr-core/src/export.rs

+        let mut module = model::Module::default();
+        module.nodes.reserve(hugr.node_count());


Add a Module::with_capacity instead?

Nodes, terms and regions have different capacities, so that'd be a with_capacity with quite a few parameters (all of the same type, don't mix them up!).

Then maybe a reserve_nodes method instead, that can be documented instead of relying on knowing how the Model works internally.

There isn't really an internally. It's plain old data. Everything is pub.

hugr-core/src/export.rs

aborgna-q · 2024-10-04T14:24:09Z

hugr-core/src/export.rs

+        }
+    }
+
+    pub fn export_root(&mut self) {


Why is the logic for the root module of the Hugr different from the node's?

This seems to assume the hugr root is a Module.
As far as I understand, passing a DFG-rotted hugr here will fail when calling self.export_node(child) on the input / output nodes.

Some root Optypes like Case and ExitBlock should be detected early and rejected here.

The core root module node is exported not as a node but as a region. I was not aware that there can be Hugrs whose roots are not Modules...

I propose we postpone this: #1554

hugr-core/src/export.rs

zrho added 10 commits October 2, 2024 11:19

hugr-model import, export and text format draft.

c15b2e1

Import CFGs.

8f3773f

Made the model import/export test into an insta test.

4e49b90

Import tail-loop and cond by inferring types from port types.

b7305fc

Merge branch 'main' into zrho/model-import

2d2a273

Fixed lints.

55624b6

Feature gate the model in hugr-core.

2a8c34f

Added node and region types.

9eb6a6f

Print type hint.

deaeea8

Add README to hugr-model

1cf28df

zrho added 6 commits October 2, 2024 14:36

Lints.

26441df

Added cfgs and tail-loops to tests and fixed bugs.

ecaf640

Added cond node to model import/export test.

6c3703a

Split up the test model files into smaller files.

cff376a

load-func in model import/export test.

df958c9

Removed resolved TODOs.

ec1736f

zrho marked this pull request as ready for review October 3, 2024 09:00

zrho requested review from aborgna-q, cqc-alec, ss2165 and a team as code owners October 3, 2024 09:00

ss2165 reviewed Oct 3, 2024

View reviewed changes

hugr-model/src/v0/mod.rs Show resolved Hide resolved

zrho mentioned this pull request Oct 3, 2024

feat: Allow extension operations to have child graphs #1546

Open

ss2165 reviewed Oct 3, 2024

View reviewed changes

doug-q reviewed Oct 3, 2024

View reviewed changes

hugr-core/tests/model.rs Outdated Show resolved Hide resolved

doug-q reviewed Oct 3, 2024

View reviewed changes

hugr-model/src/v0/mod.rs Outdated Show resolved Hide resolved

aborgna-q reviewed Oct 3, 2024

View reviewed changes

zrho and others added 2 commits October 3, 2024 15:32

Feature gate on tests via Cargo.toml

e82b99c

Update hugr-model/Cargo.toml

1d7a9b2

Co-authored-by: Agustín Borgna <[email protected]>

zrho and others added 3 commits October 3, 2024 15:40

Update hugr-model/README.md

9c34183

Co-authored-by: Agustín Borgna <[email protected]>

Update hugr-model/README.md

3573d44

Co-authored-by: Agustín Borgna <[email protected]>

Moved dependency features to Cargo.toml's of the individual crates.

ea5f734

Also added hugr model to change-filters.yaml

cqc-alec reviewed Oct 3, 2024

View reviewed changes

hugr-model/src/v0/text/print.rs Show resolved Hide resolved

hugr-model/src/v0/text/parse.rs Show resolved Hide resolved

hugr-model/src/v0/text/parse.rs Show resolved Hide resolved

zrho added 2 commits October 3, 2024 16:02

Various naming improvements, more explicit define_index! macro.

39a835b

More doc strings, feature marked as unstable, and some renamed fields.

280ef39

ss2165 mentioned this pull request Oct 3, 2024

HUGR Model and binary serialisation format. #1433

Open

13 tasks

ss2165 reviewed Oct 4, 2024

View reviewed changes

hugr-core/src/import.rs Show resolved Hide resolved

hugr-core/src/import.rs Outdated Show resolved Hide resolved

hugr-core/src/import.rs Show resolved Hide resolved

hugr-core/src/import.rs Outdated Show resolved Hide resolved

hugr-core/src/import.rs Outdated Show resolved Hide resolved

ss2165 reviewed Oct 4, 2024

View reviewed changes

Update hugr-core/src/import.rs

e18066d

Co-authored-by: Seyon Sivarajah <[email protected]>

aborgna-q reviewed Oct 4, 2024

View reviewed changes

zrho added 3 commits October 4, 2024 16:10

Fixed bugs with extension sets, and small improvements.

8070fd1

Nits

14e0e1e

Remove println left in from debugging.

706b7ac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Draft for `hugr-model` with export, import, parsing and pretty printing #1542

feat: Draft for `hugr-model` with export, import, parsing and pretty printing #1542

zrho commented Oct 2, 2024

codecov bot commented Oct 2, 2024 •

edited

Loading

ss2165 left a comment

aborgna-q left a comment •

edited by zrho

Loading

cqc-alec left a comment

ss2165 left a comment

ss2165 left a comment

ss2165 Oct 4, 2024

zrho Oct 4, 2024 •

edited

Loading

aborgna-q Oct 4, 2024

zrho Oct 4, 2024

aborgna-q Oct 4, 2024

zrho Oct 4, 2024

aborgna-q Oct 4, 2024

zrho Oct 4, 2024

zrho Oct 4, 2024

		let mut module = model::Module::default();
		module.nodes.reserve(hugr.node_count());

feat: Draft for hugr-model with export, import, parsing and pretty printing #1542

Are you sure you want to change the base?

feat: Draft for hugr-model with export, import, parsing and pretty printing #1542

Conversation

zrho commented Oct 2, 2024

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

ss2165 left a comment

Choose a reason for hiding this comment

aborgna-q left a comment • edited by zrho Loading

Choose a reason for hiding this comment

cqc-alec left a comment

Choose a reason for hiding this comment

ss2165 left a comment

Choose a reason for hiding this comment

ss2165 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zrho Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: Draft for `hugr-model` with export, import, parsing and pretty printing #1542

feat: Draft for `hugr-model` with export, import, parsing and pretty printing #1542

codecov bot commented Oct 2, 2024 •

edited

Loading

aborgna-q left a comment •

edited by zrho

Loading

zrho Oct 4, 2024 •

edited

Loading