feat!: `Extension` requires a version #1367

ss2165 · 2024-07-25T15:35:52Z

BREAKING CHANGE: Extension::new now requires a Version argument. Extension::new_with_reqs replaced with with_reqs which takes self (chain with new).

codecov · 2024-07-25T15:39:06Z

Codecov Report

Attention: Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.71%. Comparing base (1218d21) to head (8824b7a).
Report is 2 commits behind head on main.

Files	Patch %	Lines
hugr-core/src/extension.rs	98.07%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1367      +/-   ##
==========================================
+ Coverage   87.64%   87.71%   +0.06%     
==========================================
  Files         113      113              
  Lines       20030    20059      +29     
  Branches    17764    17793      +29     
==========================================
+ Hits        17556    17594      +38     
+ Misses       1699     1691       -8     
+ Partials      775      774       -1

Flag	Coverage Δ
python	`91.52% <ø> (ø)`

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 · 2024-07-25T16:13:12Z

hugr-core/src/extension.rs

@@ -3,6 +3,7 @@
 //! TODO: YAML declaration and parsing. This should be similar to a plugin
 //! system (outside the `types` module), which also parses nested [`OpDef`]s.

+pub use semver::Version;


pub so downstream crates don't need to add a semver dependency

BREAKING CHANGE: `Extension::new` now requires a `Version` argument. `Extension::new_with_reqs` replaced with `with_reqs` which takes `self` (chain with `new`).

acl-cqc

Generally great, thanks @ss2165, tho a few questions in comments.

acl-cqc · 2024-07-29T10:51:50Z

hugr-core/src/extension.rs

@@ -57,10 +58,13 @@ impl ExtensionRegistry {
    ) -> Result<Self, ExtensionRegistryError> {
        let mut exts = BTreeMap::new();
        for ext in value.into_iter() {
+            let ext_v = ext.version().clone();


This looks fine, but does look like you could make an empty ExtensionRegistry and then just do value.into_iter().for_each(|e| reg.register(e))

acl-cqc · 2024-07-29T10:53:20Z

hugr-core/src/extension.rs

+            btree_map::Entry::Occupied(mut prev) => {
+                if prev.get().version() < extension.version() {
+                    *prev.get_mut() = extension;
+                }


Might be worth a note that ideally we would check the extensions were identical but this is not possible (no Eq)

acl-cqc · 2024-07-29T10:54:18Z

hugr-core/src/extension.rs

            types: Default::default(),
            values: Default::default(),
            operations: Default::default(),
        }
    }

+    /// Creates a new extension with the given name and requirements.


Update this. Maybe better to keep the old requirements and merge in the new ones, rather than replace/drop the old ones, too

acl-cqc · 2024-07-29T10:56:41Z

hugr-core/src/extension/simple_op.rs

@@ -319,7 +319,7 @@ mod test {

    lazy_static! {
        static ref EXT: Extension = {
-            let mut e = Extension::new(EXT_ID.clone());
+            let mut e = Extension::new(EXT_ID.clone(), semver::Version::new(0, 1, 0));


consider a test-only impl Extension { fn new(id: ExtensionId) {Self::new(id, TEST_VERSION)} }

acl-cqc · 2024-07-29T10:57:47Z

hugr-core/src/std_extensions/ptr.rs

@@ -80,9 +80,12 @@ pub const PTR_TYPE_ID: TypeName = TypeName::new_inline("ptr");
 const TYPE_PARAMS: [TypeParam; 1] = [TypeParam::Type {
    b: TypeBound::Copyable,
 }];
+/// Extension version.
+pub const VERSION: semver::Version = semver::Version::new(0, 1, 0);


Yeah, I'm less sure what's appropriate for all the std / non-test extensions already in the repo. I guess this is fine.

acl-cqc · 2024-07-29T10:58:37Z

hugr-passes/src/merge_bbs.rs

@@ -178,7 +178,7 @@ mod test {
    }

    fn extension() -> Extension {
-        let mut e = Extension::new(EXT_ID);
+        let mut e = Extension::new(EXT_ID, hugr_core::extension::Version::new(0, 1, 0));


super-optional-nit, we're not terribly consistent about using hugr_core::extension::Version vs semver::Version. Possibly this one is right and the others would be better done this way too...

I use semver inside the hugr-core crate and hugr_core:: outside

acl-cqc · 2024-07-29T11:03:20Z

hugr-core/src/extension.rs

            )),
            btree_map::Entry::Vacant(ve) => Ok(ve.insert(extension)),
        }
    }

+    /// Registers a new extension to the registry, keeping most up to date if extension exists.
+    ///
+    /// If extension IDs match, the extension with the higher version is kept.


I note we are not really doing anything with Semver here (it could be any totally-ordered thing), and I wonder whether this should be the place. Replacing an extension with a breaking-change-upgrade could be problematic; should we

have a boolean flag here, allow breaking-change upgrades

keep multiple major versions of the extension (a single minor version of each)? I think this will be too difficult because then we'd have to version every reference to an extension.

Error on any breaking-change upgrade

Do something at Hugr validation time. In which case maybe just the checks we have are sufficient, so maybe the right answer is to do nothing, i.e. leave PR as it stands....

Yes I think I prefer to keep the logic here simple and flexible and deal with valid
updates etc. elsewhere.

Maybe I should add a "remove" function so the registry is actually fully flexible, and
this is just a utility for update-only changes?

added remove

## 🤖 New release * `hugr`: 0.10.0 -> 0.11.0 * `hugr-core`: 0.7.0 -> 0.8.0 * `hugr-passes`: 0.6.2 -> 0.7.0 * `hugr-cli`: 0.3.0 -> 0.4.0 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.11.0 (2024-08-12) ### Bug Fixes - [**breaking**] BasicBlockExits should not be `OpTag::DataflowParent` ([#1409](#1409)) ### Documentation - Clarify CustomConst::equal_consts ([#1396](#1396)) ### Features - [**breaking**] Serialised extensions ([#1371](#1371)) - Serialised standard extensions ([#1377](#1377)) - [**breaking**] Update remaining builder methods to "infer by default" ([#1386](#1386)) - Add Eq op to logic extension ([#1398](#1398)) - Improve error message on failed custom op validation ([#1416](#1416)) - [**breaking**] `Extension` requires a version ([#1367](#1367)) </blockquote> ## `hugr-core` <blockquote> ## 0.8.0 (2024-08-12) ### Bug Fixes - [**breaking**] BasicBlockExits should not be `OpTag::DataflowParent` ([#1409](#1409)) ### Documentation - Clarify CustomConst::equal_consts ([#1396](#1396)) ### Features - [**breaking**] `Extension` requires a version ([#1367](#1367)) - [**breaking**] Serialised extensions ([#1371](#1371)) - Serialised standard extensions ([#1377](#1377)) - [**breaking**] Update remaining builder methods to "infer by default" ([#1386](#1386)) - Add Eq op to logic extension ([#1398](#1398)) - Improve error message on failed custom op validation ([#1416](#1416)) </blockquote> ## `hugr-passes` <blockquote> ## 0.7.0 (2024-08-12) ### Features - [**breaking**] `Extension` requires a version ([#1367](#1367)) </blockquote> ## `hugr-cli` <blockquote> ## 0.4.0 (2024-08-12) ### Features - Serialised standard extensions ([#1377](#1377)) - Validate with extra extensions and packages ([#1389](#1389)) - [**breaking**] Move mermaid to own sub-command ([#1390](#1390)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Craig Roy <craig.roy@cambridgequantum.com>

ss2165 requested a review from a team as a code owner July 25, 2024 15:35

ss2165 requested a review from acl-cqc July 25, 2024 15:35

ss2165 removed the request for review from acl-cqc July 25, 2024 15:39

ss2165 marked this pull request as draft July 25, 2024 15:39

ss2165 force-pushed the ss/ext-version branch from 6c87e96 to 24b35c9 Compare July 25, 2024 16:11

ss2165 marked this pull request as ready for review July 25, 2024 16:11

ss2165 requested a review from acl-cqc July 25, 2024 16:12

ss2165 commented Jul 25, 2024

View reviewed changes

ss2165 added 2 commits July 26, 2024 15:38

feat!: Extension requires a version

3cbb454

BREAKING CHANGE: `Extension::new` now requires a `Version` argument. `Extension::new_with_reqs` replaced with `with_reqs` which takes `self` (chain with `new`).

allow updating extensions with higher version in registry.

09a9e25

ss2165 force-pushed the ss/ext-version branch from 24b35c9 to 09a9e25 Compare July 26, 2024 14:38

acl-cqc approved these changes Jul 29, 2024

View reviewed changes

ss2165 added 4 commits July 29, 2024 13:28

feat: registry remove extension method

0bdd95d

feat: with_reqs merges extension sets

8606db2

refactor: simplify ExtensionRegistry::try_new

0817170

test: test-only versioned extension constructor

8824b7a

ss2165 added this pull request to the merge queue Jul 29, 2024

Merged via the queue into main with commit b2d4013 Jul 29, 2024
22 checks passed

ss2165 deleted the ss/ext-version branch July 29, 2024 12:51

hugrbot mentioned this pull request Jul 29, 2024

chore: release #1376

Merged

hugrbot mentioned this pull request Aug 13, 2024

chore: release #1420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: `Extension` requires a version #1367

feat!: `Extension` requires a version #1367

ss2165 commented Jul 25, 2024

codecov bot commented Jul 25, 2024 •

edited

Loading

ss2165 Jul 25, 2024

acl-cqc left a comment

acl-cqc Jul 29, 2024

acl-cqc Jul 29, 2024

acl-cqc Jul 29, 2024

acl-cqc Jul 29, 2024

acl-cqc Jul 29, 2024

acl-cqc Jul 29, 2024

ss2165 Jul 29, 2024

acl-cqc Jul 29, 2024

ss2165 Jul 29, 2024

ss2165 Jul 29, 2024

feat!: Extension requires a version #1367

feat!: Extension requires a version #1367

Conversation

ss2165 commented Jul 25, 2024

codecov bot commented Jul 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

feat!: `Extension` requires a version #1367

feat!: `Extension` requires a version #1367

codecov bot commented Jul 25, 2024 •

edited

Loading