feat(tket2-hseries)!: Add `tket2.wasm` extension #737

doug-q · 2024-12-27T11:06:16Z

BREAKING CHANGE: Change the signature of FutureOpBuilder::add_read to return a Wire instead of a [Wire;1]

BREAKING CHANGE: Change the signature of `FutureOpBuilder::add_read` to return a `Wire` instead of a `[Wire;1]`

codecov · 2024-12-27T11:09:25Z

Codecov Report

Attention: Patch coverage is 76.85185% with 100 lines in your changes missing coverage. Please review.

Project coverage is 82.33%. Comparing base (0c6101b) to head (85b75ab).

Files with missing lines	Patch %	Lines
tket2-hseries/src/extension/wasm.rs	76.58%	83 Missing and 17 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #737      +/-   ##
==========================================
- Coverage   82.62%   82.33%   -0.29%     
==========================================
  Files          62       63       +1     
  Lines        7234     7661     +427     
  Branches     6978     7405     +427     
==========================================
+ Hits         5977     6308     +331     
- Misses        883      963      +80     
- Partials      374      390      +16

Flag	Coverage Δ
python	`82.42% <ø> (ø)`
rust	`82.33% <76.85%> (-0.30%)`	⬇️

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.

lmondada

Thanks @doug-q! It's always very fun to read your code, I learn a lot.

I don't have much useful feedback, other than that your design seems perfectly sound!

lmondada · 2025-01-06T09:11:15Z

tket2-hseries/src/extension/wasm.rs

+lazy_static! {
+    /// The `tket2.wasm` extension.
+    pub static ref EXTENSION: Arc<Extension> = Extension::new_arc(EXTENSION_ID, EXTENSION_VERSION, |ext, ext_ref| {
+        ext.add_requirements(ExtensionSet::from_iter([futures::EXTENSION_ID, PRELUDE_ID]));
+        add_wasm_type_defs(ext, ext_ref).unwrap();
+        WasmOpDef::load_all_ops(ext, ext_ref, ).unwrap();
+    });
+
+    /// A [Weak] reference to the `tket2.wasm` op.
+    pub static ref EXTENSION_REF: Weak<Extension> = Arc::downgrade(&EXTENSION);
+
+    /// Extension registry including the "tket2.wasm" extension and
+    /// dependencies.
+    pub static ref REGISTRY: ExtensionRegistry = ExtensionRegistry::new([
+        EXTENSION.to_owned(),
+        futures::EXTENSION.to_owned(),
+        PRELUDE.to_owned()
+    ]);
+
+    /// The name of the `tket2.wasm.module` type.
+    pub static ref MODULE_TYPE_NAME: SmolStr = SmolStr::new_inline("module");
+    /// The name of the `tket2.wasm.context` type.
+    pub static ref CONTEXT_TYPE_NAME: SmolStr = SmolStr::new_inline("context");
+    /// The name of the `tket2.wasm.func` type.
+    pub static ref FUNC_TYPE_NAME: SmolStr = SmolStr::new_inline("func");
+
+    /// The [TypeParam] of `tket2.wasm.lookup` specifying the name of the function.
+    pub static ref NAME_PARAM: TypeParam = TypeParam::String;
+    /// The [TypeParam] of various types and ops specifying the input signature of a function.
+    pub static ref INPUTS_PARAM: TypeParam = TypeParam::List { param: Box::new(TypeBound::Any.into()) };
+    /// The [TypeParam] of various types and ops specifying the output signature of a function.
+    pub static ref OUTPUTS_PARAM: TypeParam = TypeParam::List { param: Box::new(TypeBound::Any.into()) };
+}


It might be that cargo fmt-ing this without the macro will break up some of the lines that are looking quite long at the moment.

Good tip, will do

lmondada · 2025-01-06T09:18:52Z

tket2-hseries/src/extension/wasm.rs

+///
+/// This will fail if `arg` is of non-type kind (e.g. String),
+/// or if `arg` contains row variables.
+/// TODO move this to `impl TypeArg` in `hugr-core`


Are you leaving this here for the moment because you do not want to make changes to hugr-core right now?

It's more that I don't to condition this PR on a hugr release. I'll update to reference CQCL/hugr#1837

lmondada · 2025-01-06T09:51:47Z

tket2-hseries/src/extension/wasm.rs

+                        INPUTS_PARAM.to_owned(),
+                        OUTPUTS_PARAM.to_owned(),
+                    ],
+                    Signature::new(vec![module_type], vec![func_type]),


I'm a noob when it comes to define (parametrised) type signatures, so take this with a grain of salt. I notice that Self::lookup and Self::call use different ways to define the signatures returned, even though superficially they seem quite similar to me (Signature::new vs FuncValueType::new, Type vs WasmType etc)...

It would make it more readable if both code blocks were as similar as possible. If you can't use the same function calls & types for whatever reason, then maybe just put the respective function signatures in a comment -- it took me awhile to parse what they meant and had to refer several times back to the module docs (which were supremely useful btw).

I'll add comments as you suggest. re Signature + FuncValueType: the difference is that FuncValueType is allowed to have row variables in its input/output rows, while Signature must not (because we need to be able to count its ports).

Lookup returns a FuncValueType, while the callee of Call must have a signature (so that call can have a matching signature.

doug-q · 2025-01-07T09:01:12Z

It's not urgent for us to merge this. Before we merge:

I have committed to some revisions in response to Lucas review
I plan to add test cases for WasmType conversions before merging.
I would also like @mark-koch to sanity check the extension design

mark-koch · 2025-01-08T10:22:20Z

The extension design looks good to me 👍

Just to clarify, the WasmType::Context is to track global state between wasm calls, right? So it's fine to call GetContext multiple times if you don't care about preserving that state?

doug-q · 2025-01-08T10:29:25Z

The extension design looks good to me 👍

Just to clarify, the WasmType::Context is to track global state between wasm calls, right? So it's fine to call GetContext multiple times if you don't care about preserving that state?

I had seen it as ordering all wasm calls (it's linear), but yes tracking global state too. For GetContext to work (i.e. return a Some) that context (i.e. corresponding to usize passed to GetContext) must be currently disposed. I expect global state to be preserved between DisposeContexts and GetContexts.

ss2165

Looks good, add to

tket2/tket2-hseries/src/bin/tket2-hseries.rs

Lines 9 to 10 in 99c7678

    
           CliArgs::GenExtensions(args) => { 
        
               let reg = ExtensionRegistry::new([

and regenerate extensions?

tket2-hseries/src/extension/wasm.rs

ss2165

I would strongly prefer to separate out the breaking change in to a separate commit to main

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

feat!(tket2-hseries): Add tket2.wasm extension

bc7385a

BREAKING CHANGE: Change the signature of `FutureOpBuilder::add_read` to return a `Wire` instead of a `[Wire;1]`

doug-q requested a review from a team as a code owner December 27, 2024 11:06

doug-q requested a review from lmondada December 27, 2024 11:06

doug-q changed the title ~~feat!(tket2-hseries): Add tket2.wasm extension~~ feat(tket2-hseries)!: Add tket2.wasm extension Dec 27, 2024

coverage

99c7678

doug-q force-pushed the doug/wasm branch from 7a68bf3 to 99c7678 Compare December 27, 2024 11:28

lmondada approved these changes Jan 6, 2025

View reviewed changes

doug-q linked an issue Jan 7, 2025 that may be closed by this pull request

Add Wasm extension #739

Open

doug-q mentioned this pull request Jan 7, 2025

Add support for wasm CQCL/guppylang#755

Open

doug-q requested a review from ss2165 January 8, 2025 10:29

ss2165 reviewed Jan 8, 2025

View reviewed changes

tket2-hseries/src/extension/wasm.rs Outdated Show resolved Hide resolved

tket2-hseries/src/extension/wasm.rs Outdated Show resolved Hide resolved

ss2165 requested changes Jan 8, 2025

View reviewed changes

doug-q and others added 2 commits January 8, 2025 13:36

Update tket2-hseries/src/extension/wasm.rs

f2bd291

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

Update tket2-hseries/src/extension/wasm.rs

85b75ab

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tket2-hseries)!: Add `tket2.wasm` extension #737

feat(tket2-hseries)!: Add `tket2.wasm` extension #737

doug-q commented Dec 27, 2024

codecov bot commented Dec 27, 2024 •

edited

Loading

lmondada left a comment •

edited

Loading

lmondada Jan 6, 2025

doug-q Jan 7, 2025

lmondada Jan 6, 2025

doug-q Jan 7, 2025

lmondada Jan 6, 2025

doug-q Jan 7, 2025

doug-q commented Jan 7, 2025

mark-koch commented Jan 8, 2025

doug-q commented Jan 8, 2025

ss2165 left a comment

ss2165 left a comment

	CliArgs::GenExtensions(args) => {
	let reg = ExtensionRegistry::new([

feat(tket2-hseries)!: Add tket2.wasm extension #737

Are you sure you want to change the base?

feat(tket2-hseries)!: Add tket2.wasm extension #737

Conversation

doug-q commented Dec 27, 2024

codecov bot commented Dec 27, 2024 • edited Loading

Codecov Report

lmondada left a comment • edited Loading

Choose a reason for hiding this comment

lmondada Jan 6, 2025

Choose a reason for hiding this comment

doug-q Jan 7, 2025

Choose a reason for hiding this comment

lmondada Jan 6, 2025

Choose a reason for hiding this comment

doug-q Jan 7, 2025

Choose a reason for hiding this comment

lmondada Jan 6, 2025

Choose a reason for hiding this comment

doug-q Jan 7, 2025

Choose a reason for hiding this comment

doug-q commented Jan 7, 2025

mark-koch commented Jan 8, 2025

doug-q commented Jan 8, 2025

ss2165 left a comment

Choose a reason for hiding this comment

ss2165 left a comment

Choose a reason for hiding this comment

feat(tket2-hseries)!: Add `tket2.wasm` extension #737

feat(tket2-hseries)!: Add `tket2.wasm` extension #737

codecov bot commented Dec 27, 2024 •

edited

Loading

lmondada left a comment •

edited

Loading