Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Jan 16, 2025
Merged

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

merged 11 commits into from
Jan 16, 2025

Conversation

doug-q
Copy link
Contributor

@doug-q doug-q commented Dec 27, 2024

No description provided.

BREAKING CHANGE: Change the signature of `FutureOpBuilder::add_read` to
return a `Wire` instead of a `[Wire;1]`
@doug-q doug-q requested a review from a team as a code owner December 27, 2024 11:06
@doug-q doug-q requested a review from lmondada December 27, 2024 11:06
@doug-q doug-q changed the title feat!(tket2-hseries): Add tket2.wasm extension feat(tket2-hseries)!: Add tket2.wasm extension Dec 27, 2024
Copy link

codecov bot commented Dec 27, 2024

Codecov Report

Attention: Patch coverage is 79.06977% with 90 lines in your changes missing coverage. Please review.

Project coverage is 82.31%. Comparing base (0c0b238) to head (82f3b71).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
tket2-hseries/src/extension/wasm.rs 79.25% 74 Missing and 15 partials ⚠️
tket2-hseries/src/bin/tket2-hseries.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #737      +/-   ##
==========================================
- Coverage   82.48%   82.31%   -0.18%     
==========================================
  Files          62       63       +1     
  Lines        7310     7740     +430     
  Branches     7054     7484     +430     
==========================================
+ Hits         6030     6371     +341     
- Misses        905      979      +74     
- Partials      375      390      +15     
Flag Coverage Δ
python 82.42% <ø> (ø)
rust 82.30% <79.06%> (-0.19%) ⬇️

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.

Copy link
Contributor

@lmondada lmondada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment on lines 90 to 122
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()) };
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good tip, will do

tket2-hseries/src/extension/wasm.rs Outdated Show resolved Hide resolved
INPUTS_PARAM.to_owned(),
OUTPUTS_PARAM.to_owned(),
],
Signature::new(vec![module_type], vec![func_type]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added signatures in comments, but the syntax is quite made up. What do you think?

@doug-q
Copy link
Contributor Author

doug-q commented Jan 7, 2025

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

@doug-q doug-q linked an issue Jan 7, 2025 that may be closed by this pull request
@mark-koch
Copy link
Contributor

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
Copy link
Contributor Author

doug-q commented Jan 8, 2025

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.

@doug-q doug-q requested a review from ss2165 January 8, 2025 10:29
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, add to

CliArgs::GenExtensions(args) => {
let reg = ExtensionRegistry::new([
and regenerate extensions?

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

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@doug-q doug-q changed the title feat(tket2-hseries)!: Add tket2.wasm extension feat(tket2-hseries): Add tket2.wasm extension Jan 16, 2025
@doug-q doug-q requested a review from ss2165 January 16, 2025 15:59
Copy link
Member

@ss2165 ss2165 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the changes

#[case(WasmType::Context)]
#[case(WasmType::new_func(type_row![], type_row![]))]
#[case(WasmType::new_func(vec![TypeRV::new_row_var_use(0, TypeBound::Any)], vec![bool_t()]))]
fn wasm_type(#[case] wasm_t: WasmType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a formatting issue

@doug-q doug-q enabled auto-merge January 16, 2025 17:05
@doug-q doug-q added this pull request to the merge queue Jan 16, 2025
Merged via the queue into main with commit 34bdc21 Jan 16, 2025
15 checks passed
@doug-q doug-q deleted the doug/wasm branch January 16, 2025 17:10
This was referenced Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Wasm extension
4 participants