feat!: `used_extensions` calls for both ops and signatures #1739

aborgna-q · 2024-12-03T17:59:44Z

Adds methods

OpType::used_extensions(&self) -> Result<ExtensionRegistry, _>
Signature::used_extensions(&self) -> Result<ExtensionRegistry, _>
and tests these along with the code merged in feat!: Resolve OpaqueOps and CustomType extensions #1735.

Moves the code from #1735 into resolution::types_mut, and adds a (quite-similar) non-mutable version in ::types that only collects the extensions without modifying the OpType.

Fixes the resolution not exploring types inside a CustomType type arguments.

drive-by: Implement Display, Serialize, and ::new for ExtensionRegistry.
drive-by: ExtensionSet should take ids by value when inserting.
drive-by: Fix Hugr::resolve_extension_defs not scanning all the ops.

These changes were extracted from #1738.

BREAKING CHANGE: Removed ExtensionRegistry::try_new. Use new instead, and call ExtensionRegistry::validate to validate.
BREAKING CHANGE: ExtensionSet::insert and singleton take extension ids by value instead of cloning internally.

codecov · 2024-12-03T19:10:53Z

Codecov Report

Attention: Patch coverage is 82.62712% with 82 lines in your changes missing coverage. Please review.

Project coverage is 86.29%. Comparing base (4c1c6ee) to head (2c8ad5e).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/extension/resolution/types_mut.rs	75.00%	4 Missing and 31 partials ⚠️
hugr-core/src/extension.rs	71.01%	20 Missing ⚠️
hugr-core/src/extension/resolution/types.rs	92.85%	7 Missing ⚠️
hugr-core/src/extension/resolution/ops.rs	68.75%	5 Missing ⚠️
hugr-core/src/ops/custom.rs	72.72%	3 Missing ⚠️
hugr-core/src/extension/resolution/test.rs	92.00%	1 Missing and 1 partial ⚠️
hugr-core/src/hugr/rewrite/inline_dfg.rs	66.66%	0 Missing and 2 partials ⚠️
hugr-core/src/hugr/rewrite/replace.rs	0.00%	2 Missing ⚠️
hugr-core/src/hugr/validate/test.rs	77.77%	0 Missing and 2 partials ⚠️
hugr-core/src/extension/op_def.rs	75.00%	0 Missing and 1 partial ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1739      +/-   ##
==========================================
+ Coverage   86.28%   86.29%   +0.01%     
==========================================
  Files         177      179       +2     
  Lines       32087    32662     +575     
  Branches    28999    29574     +575     
==========================================
+ Hits        27687    28187     +500     
- Misses       2775     2797      +22     
- Partials     1625     1678      +53

Flag	Coverage Δ
python	`92.42% <ø> (ø)`
rust	`85.65% <82.62%> (+0.02%)`	⬆️

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.

aborgna-q · 2024-12-04T10:35:18Z

~~Coverage is quite bad. I'll add some extra tests today.~~

Done

hugr-core/src/extension.rs

hugr-core/src/extension/resolution.rs

hugr-core/src/extension/resolution/types.rs

acl-cqc · 2024-12-05T16:37:11Z

hugr-core/src/extension/resolution/types.rs

-            update_type_row_exts(node, &mut db.other_outputs, extensions, used_extensions)?;
-            for row in db.sum_rows.iter_mut() {
-                update_type_row_exts(node, row, extensions, used_extensions)?;
+            collect_type_row_exts(&db.inputs, &mut used, &mut missing);


Doesn't the signature of a DataflowBlock include all the elements of the inputs, other_outputs and sum_rows? So can't you just collect_signature_exts on the block's signature, and the same for e.g. TailLoop, Conditional, and so on, like OpaqueOp and most other blocks?

Yes, but that requires allocating a Signature, cloning all row elements, computing the concrete input/output rows, and immediately destructuring them to run this same logic.

Is this true even with Cow of signature? I mean, we're gonna be computing that signature a good few times anyway...

Hmmmm, even when we have Cow<Signature>? I mean, we are gonna be constructing that thing a few times, clearly it's gonna help to cache it in the op. OK don't add that for this PR, but if you're optimizing that anyway and could use that here...

acl-cqc · 2024-12-05T17:06:27Z

hugr-core/src/types/signature.rs

+    /// signature. The extension deltas added via [`Self::with_extension_delta`]
+    /// refer to _runtime_ extensions, which may not be in all places that
+    /// manipulate a HUGR.
+    pub fn used_extensions(&self) -> Result<ExtensionRegistry, ExtensionCollectionError> {


So the other way to do this - is to put the collect_blah_exts methods as impls of a single method in a trait, and then you can add a trait-default method (or a method taking an impl ExtensionCollectionTrait) that does this...that'd get you the two used_extensions methods that you have plus a load more with only having to write this once. (I wasn't going to suggest the trait approach until I saw this...)

This doesn't work if you change OpType::used_extensions to take the extra Option<Node> parameter I suggested elsewhere, though. So feel free to drop.

I opted for keeping all the logic in a single module, as it's short and quite similar between all the ops.

hugr-core/src/extension/resolution/types.rs

acl-cqc · 2024-12-05T17:14:39Z

hugr-core/src/extension/resolution/test.rs

+    let mut collected_exts = ExtensionRegistry::default();
+    for node in hugr.nodes() {
+        let op = hugr.get_optype(node);
+        collected_exts.extend(collect_op_extensions(Some(node), op).unwrap());


If you changed OpType::used_extensions to take the Option<Node> param as extra, which seems reasonable, then you could call that here and remove all direct calls to collect_op_(type_)extensions. Then hide those (or inline into the single caller OpType::used_extensions).

I was calling the resolve methods explicitly, but it's exactly the same as op.used_exts.
I just changed it to that.

acl-cqc · 2024-12-05T17:17:14Z

hugr-core/src/extension/resolution/types.rs

+/// - `node`: The node where the operation is located, if available.
+///   This is used to provide context in the error message.
+/// - `op`: The operation to collect the extensions from.
+pub fn collect_op_types_extensions(


Should this be pub? The nomenclature is a bit confusing here - it feels equivalent to Signature::used_extensions but actually it's only one part of OpType::used_extensions. However the other collect things take two &muts. Should one really use this directly?

It's hidden inside the private types module. But I'll add the (crate) for clarity.

acl-cqc · 2024-12-05T17:19:19Z

hugr-core/src/extension/resolution/types_mut.rs

+//! Resolve weak links inside `CustomType`s in an optype's signature, while
+//! collecting all used extensions.
+//!
+//! For a non-mutating option see [`super::collect_op_types_extensions`].


I reckon the pub function here should be OpType::used_extensions.

It's a private module. (Otherwise the docs check would complain)

Ah ok, yes I missed that. Ok I'll hold some fire there then ;)

…result

hugrbot · 2024-12-09T10:56:07Z

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary


--- 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.37.0/src/lints/inherent_method_missing.ron

Failed in:
ExtensionRegistry::try_new, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/extension.rs:58

--- failure pub_module_level_const_missing: pub module-level const is missing ---

Description:
A public const is missing, renamed, or changed from const to static.
      ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
     impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.37.0/src/lints/pub_module_level_const_missing.ron

Failed in:
EMPTY_REG in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/extension.rs:209

acl-cqc

Generally - great :). I think all of these comments are now small and local, so looks good and thanks @aborgna-q , let's get it in :)

I think there is a bit of an issue that ideally one would like Hugr::validate to also validate the registry (if not cached in that AtomicBool), but that requires passing an &mut ExtensionRegistry which is gonna require changing everywhere (and annoyingly). Otherwise you could add methods to validate immutably (i.e. without updating the cached bool), I guess....

acl-cqc · 2024-12-09T17:26:53Z

hugr-core/src/extension.rs

    }

    /// Delete an extension from the registry and return it if it was present.
    pub fn remove_extension(&mut self, name: &ExtensionId) -> Option<Arc<Extension>> {
-        self.0.remove(name)
+        // Clear the valid flag so that the registry is re-validated.


Most nitty super-nit ever: these two lines after the removal, for consistency with the other operations :-) :-D

remove returns the Option<Arc<Ext>>, so moving it requires declaring local vars..

acl-cqc · 2024-12-09T17:29:32Z

hugr-core/src/extension.rs

+    /// A flag indicating whether the current set of extensions has been
+    /// validated.
+    ///
+    /// This is used to avoid re-validating the extensions every time they are


Suggested change

/// This is used to avoid re-validating the extensions every time they are

/// This is used to avoid re-validating the extensions every time the registry is

(I think - if you grab an extension out of the registry, you can use it without validating the registry; it's only when the registry itself is passed to validate a Hugr, say. Of course this is a subtlety of how we interpret "use" of an extension vs "use" of the regstry, so feel free to be more explicit - "every time a Hugr is validated against the registry", perhaps, although that's a bit more of a mouthful)

hugr-core/src/extension.rs

acl-cqc · 2024-12-09T17:33:39Z

hugr-core/src/extension.rs

 /// An Extension Registry containing no extensions.
-pub const EMPTY_REG: ExtensionRegistry = ExtensionRegistry(BTreeMap::new());
+pub static EMPTY_REG: ExtensionRegistry = ExtensionRegistry {


random driveby, feel free to ignore - could this could be a const fn, to which you could add Extensions yourself? (and then static EMPTY_REG: ExtensionRegistry = ExtensionRegistry::new_empty(); or similar)

ExtensionRegistry now has internal mutability, so it should not be used as a const.
(it does compile, but using it just raises linting errors everywhere, with good reason)

hugr-core/src/extension/resolution/types.rs

hugr-core/src/extension/resolution/ops.rs

acl-cqc · 2024-12-09T18:13:49Z

hugr-core/src/extension/resolution/types.rs

-            update_type_row_exts(node, &mut db.other_outputs, extensions, used_extensions)?;
-            for row in db.sum_rows.iter_mut() {
-                update_type_row_exts(node, row, extensions, used_extensions)?;
+            collect_type_row_exts(&db.inputs, &mut used, &mut missing);


Hmmmm, even when we have Cow<Signature>? I mean, we are gonna be constructing that thing a few times, clearly it's gonna help to cache it in the op. OK don't add that for this PR, but if you're optimizing that anyway and could use that here...

acl-cqc · 2024-12-09T18:16:53Z

hugr-core/src/hugr.rs

@@ -213,7 +213,7 @@ impl Hugr {
        //
        // This is not something we want to expose it the API, so we manually
        // iterate instead of writing it as a method.
-        for n in 0..self.node_count() {
+        for n in 0..self.graph.node_capacity() {


Yes please do! The comment above does not explain (to me) why we are using node_capacity

acl-cqc · 2024-12-09T18:27:43Z

hugr-core/src/std_extensions.rs

-    ])
-    .unwrap()
+    ]);
+    reg.validate()


Many examples here and elsewhere, I wonder if it's worth adding to ExtensionRegistry

fn validated(mut self) -> Result<Self, ....> { self.validate()?; self }

We'll call ExtRegistry::validate on each Hugr::validate as soon as #1738 gets merged, so this shouldn't be needed.

…1738) Closes #1613. Depends on #1739. Hugrs now keep an `Extensions` registry that is automatically computed when using the builder or deserializing from json (using the new `Hugr::load_json`). This set can contain unneeded extensions, but it should always be sufficient to validate the HUGR definition. Note that this is **not** runtime extensions (see #426, #1734). A big chunk of the diff is removing the extension registry when finishing building a hugr. The extension tracking is now done automatically while adding operations. drive-by: Remove unneeded `set_num_ports` call in `insert_hugr_internal`. BREAKING CHANGE: Removed `update_validate`. The hugr extensions should be resolved at load time, so we can use `validate` instead. BREAKING CHANGE: The builder `finish_hugr` function family no longer takes a registry as parameter, and the `_prelude` variants have been removed.

aborgna-q added 2 commits December 3, 2024 17:39

used_extensions calls for both ops and signatures

6d1acdb

add tests

16d0fca

aborgna-q requested a review from a team as a code owner December 3, 2024 17:59

aborgna-q requested a review from acl-cqc December 3, 2024 17:59

aborgna-q mentioned this pull request Dec 3, 2024

feat!: Hugrs now keep a ExtensionRegistry with their requirements #1738

Merged

aborgna-q added 4 commits December 4, 2024 15:01

Add more tests

3be18e9

pre-extensions in hugrs lint fixes

c0caf52

Undo breaking drive-by

8bd3799

test conditionals and errors

95e10e3

acl-cqc reviewed Dec 5, 2024

View reviewed changes

aborgna-q changed the title ~~feat: used_extensions calls for both ops and signatures~~ feat!: used_extensions calls for both ops and signatures Dec 9, 2024

aborgna-q added 2 commits December 9, 2024 10:49

Replace ExtensionRegistry::try_new with ::validate and cache the …

3220bf6

…result

doc

4aa638a

aborgna-q added 13 commits December 9, 2024 11:07

Use ExtensionSet, avoid unnecessary cloning

2f2bef5

Update typeargs in opaque and extension ops

a70e895

then_some chains

b054888

Explicit match catch-all

9cebfbc

remove redundant comments

b88fef2

suggested clarification on ignoring TypeArg::Extension

c4760e2

suggested note rephrase about runtime reqs

0aa7e84

s/update_/resolve_/

38e2e2a

fix wrong doc ref

3d2465f

test op.used_extensions directly

4b57cb6

clarify pub(crate)

48d6d9f

Merge remote-tracking branch 'origin/main' into ab/used-extensions

ea6dfe0

fix lints in main

91cc1d5

aborgna-q requested a review from acl-cqc December 9, 2024 12:05

acl-cqc approved these changes Dec 9, 2024

View reviewed changes

aborgna-q added 3 commits December 10, 2024 12:04

Add exhaustive patterns for TypeEnum too

708243a

comment on the graph nodes iterator

4935d97

doc updates

2c8ad5e

aborgna-q enabled auto-merge December 10, 2024 12:17

aborgna-q added this pull request to the merge queue Dec 10, 2024

Merged via the queue into main with commit 1443284 Dec 10, 2024
24 checks passed

aborgna-q deleted the ab/used-extensions branch December 10, 2024 12:27

hugrbot mentioned this pull request Dec 10, 2024

chore: release #1757

Closed

This was referenced Dec 11, 2024

chore: release #1761

Closed

chore: release #1778

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: `used_extensions` calls for both ops and signatures #1739

feat!: `used_extensions` calls for both ops and signatures #1739

aborgna-q commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 3, 2024 •

edited

Loading

aborgna-q commented Dec 4, 2024 •

edited

Loading

acl-cqc Dec 5, 2024

aborgna-q Dec 9, 2024

acl-cqc Dec 9, 2024

acl-cqc Dec 9, 2024

acl-cqc Dec 5, 2024

acl-cqc Dec 5, 2024

aborgna-q Dec 9, 2024

acl-cqc Dec 5, 2024

aborgna-q Dec 9, 2024

acl-cqc Dec 5, 2024

aborgna-q Dec 9, 2024

acl-cqc Dec 5, 2024

aborgna-q Dec 9, 2024

acl-cqc Dec 9, 2024

hugrbot commented Dec 9, 2024

acl-cqc left a comment

acl-cqc Dec 9, 2024

aborgna-q Dec 10, 2024

acl-cqc Dec 9, 2024

acl-cqc Dec 9, 2024

aborgna-q Dec 10, 2024

acl-cqc Dec 9, 2024

acl-cqc Dec 9, 2024

acl-cqc Dec 9, 2024

aborgna-q Dec 10, 2024

	/// This is used to avoid re-validating the extensions every time they are
	/// This is used to avoid re-validating the extensions every time the registry is

feat!: used_extensions calls for both ops and signatures #1739

feat!: used_extensions calls for both ops and signatures #1739

Conversation

aborgna-q commented Dec 3, 2024 • edited Loading

codecov bot commented Dec 3, 2024 • edited Loading

Codecov Report

aborgna-q commented Dec 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

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

hugrbot commented Dec 9, 2024

acl-cqc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat!: `used_extensions` calls for both ops and signatures #1739

feat!: `used_extensions` calls for both ops and signatures #1739

aborgna-q commented Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 3, 2024 •

edited

Loading

aborgna-q commented Dec 4, 2024 •

edited

Loading