feat!: Flatten `LeafOp` #922

aborgna-q · 2024-04-11T13:43:25Z

Moves the variants of LeafOp into OpType. Closes #817.

It seems Lift was missing from the schema, and in its place there was a TypeApplication struct. I replaced it with lift.

As an aside, the CustomOp -> ExternalOp -> {ExtensionOp, OpaqueOp} chain seems unnecessary. We should be able to merge CustomOp and ExternalOp into one, and get rid of multiple as_refs in the code.
This doesn't modify the schema, so I'll push it as a separate PR.

codecov · 2024-04-11T13:48:36Z

Codecov Report

Attention: Patch coverage is 75.67568% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 85.45%. Comparing base (75e75e8) to head (db3cdb5).

Files	Patch %	Lines
hugr/src/ops/leaf.rs	73.17%	21 Missing and 1 partial ⚠️
hugr/src/ops/custom.rs	47.82%	12 Missing ⚠️
hugr/src/algorithm/const_fold.rs	44.44%	4 Missing and 1 partial ⚠️
hugr/src/builder/dataflow.rs	57.14%	0 Missing and 3 partials ⚠️
hugr/src/builder/build_traits.rs	50.00%	1 Missing ⚠️
hugr/src/extension/simple_op.rs	0.00%	0 Missing and 1 partial ⚠️
hugr/src/hugr/serialize.rs	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #922      +/-   ##
==========================================
- Coverage   85.54%   85.45%   -0.10%     
==========================================
  Files          78       78              
  Lines       14220    14276      +56     
  Branches    14220    14276      +56     
==========================================
+ Hits        12165    12200      +35     
- Misses       1421     1442      +21     
  Partials      634      634

Flag	Coverage Δ
rust	`85.45% <75.67%> (-0.10%)`	⬇️

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.

ss2165

Looks good, thanks! Just some small queries

hugr/src/ops/leaf.rs

ss2165 · 2024-04-12T09:17:38Z

hugr/src/ops/custom.rs

@@ -28,6 +28,17 @@ pub enum ExternalOp {
    Opaque(OpaqueOp),
 }

+impl PartialEq for ExternalOp {


was this needed for this PR? looks like the non-trivial comparisons don't have any coverage either

I'll drop this, and move the discussion to the other PR
(#923 (comment))

ss2165 · 2024-04-12T09:20:38Z

hugr/src/ops/leaf.rs

+/// A user-defined operation that can be downcasted by the extensions that
+/// define it.


not sure what "downcast by extension" means?

ss2165 · 2024-04-12T09:32:52Z

Couple more things needed I think:

Comment can be deleted:

hugr/hugr-py/src/hugr/serialization/ops.py

Lines 389 to 390 in 9a87e0a

    
           # ----------------------------------------- 
        
           # --------------- LeafOp ------------------

Spec mentions LeafOp:

hugr/specification/hugr.md

Lines 2001 to 2002 in 9a87e0a

    
           | `LeafOp`       | ✱, ✱    | ✱, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          | 
        
           | `Call`         | ✱, ✱    | ✱, ✱    | 0, 0    | 1, 0       | 0, 0          | 1, 0        |          |

Can be renamed to "Leaf" with a description of what that means I think (or explicitly listing the leaf operations in the description)

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

ss2165 · 2024-04-12T13:26:11Z

hugr/src/ops/leaf.rs

+///
+/// Any custom operation can be encoded as a serializable [`OpaqueOp`].
+/// If the operation's extension is loaded in the current context, the operation
+/// can be resolved to an executable [`ExtensionOp`].


"executable" doesn't seem right, don't we just mean that the extensionop refers to it's definition in the extension?

That sounds better 👍

ss2165 · 2024-04-12T13:29:10Z

specification/hugr.md

+| `CustomOp`       | ✱, ✱    | ✱, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `Noop`       | 1, 1    | ✱, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `MakeTuple`       | ✱, 1    | ✱, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `UnpackTuple`       | 1, ✱    | ✱, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `Tag`       | 1, 1    | ✱, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |
+| `Lift`       | ✱, ✱    | ✱, ✱    | 0, 0    | 0, 0       | 0, 0          | 1, 0        |          |


formatetd the whole table:

| Node type | `Value` | `Order` | `Const` | `Function` | `ControlFlow` | `Hierarchy` | Children | |----------------|---------|---------|---------|------------|---------------|-------------|----------| | Root | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 0, ✱ | | | `FuncDefn` | 0, 0 | 0, 0 | 0, 0 | 0, * | 0, 0 | 1, + | DSG | | `FuncDecl` | 0, 0 | 0, 0 | 0, 0 | 0, * | 0, 0 | 1, 0 | | | `AliasDefn` | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `AliasDecl` | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `Const` | 0, 0 | 0, 0 | 0, ✱ | 0, 0 | 0, 0 | 1, 0 | | | `LoadConstant` | 0, 1 | +, ✱ | 1, 0 | 0, 0 | 0, 0 | 1, 0 | | | `Input` | 0, ✱ | 0, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `Output` | ✱, 0 | ✱, 0 | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `Call` | ✱, ✱ | ✱, ✱ | 0, 0 | 1, 0 | 0, 0 | 1, 0 | | | `DFG` | ✱, ✱ | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, + | DSG | | `CFG` | ✱, ✱ | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, + | CSG | | `DFB` | 0, 0 | 0, 0 | 0, 0 | 0, 0 | ✱, ✱ | 1, + | DSG | | `Exit` | 0, 0 | 0, 0 | 0, 0 | 0, 0 | +, 0 | 1, 0 | | | `TailLoop` | ✱, ✱ | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, + | DSG | | `Conditional` | ✱, ✱ | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, + | `Case` | | `Case` | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 0, 0 | 1, + | DSG | | `CustomOp` | ✱, ✱ | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `Noop` | 1, 1 | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `MakeTuple` | ✱, 1 | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `UnpackTuple` | 1, ✱ | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `Tag` | 1, 1 | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, 0 | | | `Lift` | ✱, ✱ | ✱, ✱ | 0, 0 | 0, 0 | 0, 0 | 1, 0 | |

http://markdowntable.com/

Followup of #922 . Drops `ExternalOp` and makes `CustomOp` an enum with two boxed variants for `OpaqueOp` and `ExtensionOp`. The schema remains the same. --------- Co-authored-by: Seyon Sivarajah <[email protected]>

## 🤖 New release * `hugr`: 0.3.0-alpha.1 -> 0.3.0-alpha.2 (⚠️ API breaking changes) ### ⚠️ `hugr` breaking changes ``` --- failure enum_missing: pub enum removed or renamed --- Description: A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself 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.30.0/src/lints/enum_missing.ron Failed in: enum hugr::ops::custom::ExternalOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops/custom.rs:20 enum hugr::ops::leaf::LeafOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops/leaf.rs:21 enum hugr::ops::LeafOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops/leaf.rs:21 --- 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.30.0/src/lints/enum_variant_added.ron Failed in: variant ConstTypeError:NotMonomorphicFunction in /tmp/.tmpoZAGEw/hugr/hugr/src/ops/constant.rs:98 variant ConstTypeError:NotMonomorphicFunction in /tmp/.tmpoZAGEw/hugr/hugr/src/ops/constant.rs:98 variant SignatureError:CallIncorrectlyAppliesType in /tmp/.tmpoZAGEw/hugr/hugr/src/extension.rs:172 --- 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.30.0/src/lints/enum_variant_missing.ron Failed in: variant InterGraphEdgeError::InvalidConstSrc, previously in file /tmp/.tmpZslYkR/hugr/src/hugr/validate.rs:774 variant OpType::LeafOp, previously in file /tmp/.tmpZslYkR/hugr/src/ops.rs:30 variant SignatureError::TypeApplyIncorrectCache, previously in file /tmp/.tmpZslYkR/hugr/src/extension.rs:171 variant EdgeKind::Static, previously in file /tmp/.tmpZslYkR/hugr/src/types.rs:44 variant ConstTypeError::FunctionTypeMissing, previously in file /tmp/.tmpZslYkR/hugr/src/ops/constant.rs:99 variant ConstTypeError::FunctionTypeMissing, previously in file /tmp/.tmpZslYkR/hugr/src/ops/constant.rs:99 --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn 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.30.0/src/lints/inherent_method_missing.ron Failed in: OpType::as_leaf_op, previously in file /tmp/.tmpZslYkR/hugr/src/ops.rs:103 OpType::is_leaf_op, previously in file /tmp/.tmpZslYkR/hugr/src/ops.rs:103 --- failure method_parameter_count_changed: pub method parameter count changed --- Description: A publicly-visible method now takes a different number of parameters. ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.30.0/src/lints/method_parameter_count_changed.ron Failed in: hugr::types::PolyFuncType::validate now takes 2 parameters instead of 3, in /tmp/.tmpoZAGEw/hugr/hugr/src/types/poly_func.rs:76 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself 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.30.0/src/lints/struct_missing.ron Failed in: struct hugr::ops::leaf::TypeApplication, previously in file /tmp/.tmpZslYkR/hugr/src/ops/leaf.rs:82 ``` <details><summary>Changelog</summary> <blockquote> ## 0.3.0-alpha.2 (2024-04-15) ### Documentation - Specify direct children in `HugrView::children` ([#921](#921)) - Add logo svg to readme and spec ([#925](#925)) ### Features - [**breaking**] No polymorphic closures ([#906](#906)) - [**breaking**] Flatten `LeafOp` ([#922](#922)) ### Refactor - Combine ExtensionSolutions (no separate closure) ([#884](#884)) - [**breaking**] Merge `CustomOp` and `ExternalOp`. ([#923](#923)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Agustin Borgna <[email protected]>

🤖 I have created a release *beep* *boop* --- ## 0.1.0 (2024-04-15) ### ⚠ BREAKING CHANGES * Flatten `LeafOp` ([#922](#922)) * EdgeKind::{Static -> Const}, add new EdgeKind::Function, Type contains only monomorphic functions, remove TypeApply. * **py:** Rename package to `hugr` ([#913](#913)) ### Features * Flatten `LeafOp` ([#922](#922)) ([3598913](3598913)) * No polymorphic closures ([#906](#906)) ([b05dd6b](b05dd6b)) * **py:** Rename package to `hugr` ([#913](#913)) ([9fe65db](9fe65db)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Agustín Borgna <[email protected]>

aborgna-q added 2 commits April 11, 2024 14:31

feat: Flatten LeafOp into OpType

76c8ea4

Update pydantic schema

9a87e0a

aborgna-q requested a review from ss2165 April 11, 2024 13:43

aborgna-q mentioned this pull request Apr 11, 2024

refactor!: Merge CustomOp and ExternalOp. #923

Merged

aborgna-q changed the title ~~feat: Flatten LeafOp~~ feat!: Flatten LeafOp Apr 11, 2024

ss2165 reviewed Apr 12, 2024

View reviewed changes

aborgna-q and others added 5 commits April 12, 2024 10:35

Update hugr/src/ops/leaf.rs

a070b79

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

revert manual ExternalOp definition

0d98507

Update doc for CustomOp

a695e69

Remove old comment

de74621

Update spec

573ea53

aborgna-q requested a review from ss2165 April 12, 2024 13:24

ss2165 requested changes Apr 12, 2024

View reviewed changes

aborgna-q added 2 commits April 12, 2024 15:10

Update CustomOp doc

23b0947

Format md table in spec

db3cdb5

aborgna-q requested a review from ss2165 April 12, 2024 14:10

ss2165 approved these changes Apr 12, 2024

View reviewed changes

aborgna-q added this pull request to the merge queue Apr 12, 2024

Merged via the queue into main with commit 3598913 Apr 12, 2024
17 of 18 checks passed

aborgna-q deleted the feat/flatten-leafs branch April 12, 2024 14:17

github-actions bot mentioned this pull request Apr 12, 2024

chore: release v0.3.0-alpha.2 #898

Merged

This was referenced Apr 15, 2024

chore: release v0.3.1 #928

Merged

chore(main): release hugr-py 0.1.0 #932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Flatten `LeafOp` #922

feat!: Flatten `LeafOp` #922

aborgna-q commented Apr 11, 2024

codecov bot commented Apr 11, 2024 •

edited

Loading

ss2165 left a comment

ss2165 Apr 12, 2024

aborgna-q Apr 12, 2024

ss2165 Apr 12, 2024

ss2165 commented Apr 12, 2024

ss2165 Apr 12, 2024

aborgna-q Apr 12, 2024

ss2165 Apr 12, 2024

		/// A user-defined operation that can be downcasted by the extensions that
		/// define it.

feat!: Flatten LeafOp #922

feat!: Flatten LeafOp #922

Conversation

aborgna-q commented Apr 11, 2024

codecov bot commented Apr 11, 2024 • edited Loading

Codecov Report

ss2165 left a comment

Choose a reason for hiding this comment

ss2165 Apr 12, 2024

Choose a reason for hiding this comment

aborgna-q Apr 12, 2024

Choose a reason for hiding this comment

ss2165 Apr 12, 2024

Choose a reason for hiding this comment

ss2165 commented Apr 12, 2024

ss2165 Apr 12, 2024

Choose a reason for hiding this comment

aborgna-q Apr 12, 2024

Choose a reason for hiding this comment

ss2165 Apr 12, 2024

Choose a reason for hiding this comment

feat!: Flatten `LeafOp` #922

feat!: Flatten `LeafOp` #922

codecov bot commented Apr 11, 2024 •

edited

Loading