Skip to content

wasi-nn: refactor to allow preview2 access#6821

Merged
abrown merged 10 commits intobytecodealliance:mainfrom
abrown:wasi-nn-in-wit
Aug 16, 2023
Merged

wasi-nn: refactor to allow preview2 access#6821
abrown merged 10 commits intobytecodealliance:mainfrom
abrown:wasi-nn-in-wit

Conversation

@abrown
Copy link
Member

@abrown abrown commented Aug 8, 2023

This change refactors the wasmtime-wasi-nn crate to allow access from both preview1 and preview2 ABIs. Though the wasi-nn specification has included a WIT description for some time, here we use some in-tree files until WebAssembly/wasi-nn#38 is landed. The preview2 code is not exercised anywhere yet: ideally this would be wired up once component model resources are fully implemented in Wasmtime.

@abrown abrown force-pushed the wasi-nn-in-wit branch 4 times, most recently from de9a31b to 00dfc5e Compare August 8, 2023 22:11
abrown added 2 commits August 9, 2023 08:57
This change refactors the `wasmtime-wasi-nn` crate to allow access from
both `preview1` and `preview2` ABIs. Though the `wasi-nn` specification
has included a WIT description for some time, here we use some in-tree
files until WebAssembly/wasi-nn#38 is landed.
The `preview2` code is not exercised anywhere yet: ideally this would be
wired up once component model `resource`s are fully implemented in
Wasmtime.

prtest:full
@abrown abrown marked this pull request as ready for review August 9, 2023 23:19
@abrown abrown requested review from a team as code owners August 9, 2023 23:19
@abrown abrown requested review from pchickey and removed request for a team August 9, 2023 23:19
@alexcrichton
Copy link
Member

One part of this is that it defines a hierarchy of types and then provides conversion of WIT and WITX-generated types into this hierarchy of types. That's reasonable for this crate since the set of types is quite small, but for something like wasi-common I'm not sure if that would work well (or larger proposals). In that sense it might make more sense to unconditionally generate WIT bindings (e.g. not a compile time feature) and always use the WIT types generated? That way there'd only be one conversion necessary which is WITX-to-WIT.

Also one thing to consider is that this has impl WasiNnTrait for WasiNnCtx which means that it's not going to be easily usable from embedders. Currently with bindgen! we're using something that looks like impl<T> WasiNnTrait for T where T: WasiNnView so that way if WasiNnCtx is embedded in a store somewhere the WasiNnView defines how to access it.

Also as a final thing, it looks like the implementation bodies of WIT and WITX are duplicated? Would it be possible for one to delegate to the other?

@abrown
Copy link
Member Author

abrown commented Aug 10, 2023

In that sense it might make more sense to unconditionally generate WIT bindings (e.g. not a compile time feature) and always use the WIT types generated

Yeah, I would prefer that. Is it fine if wasmtime-wasi-nn enables the component-model feature, though, forcing every user of this (e.g., the CLI) to do so too?

impl<T> WasiNnTrait for T where T: WasiNnView

I saw this over in wasi or wasi-common and didn't really "get" it. Let me look into it a bit more.

Also as a final thing, it looks like the implementation bodies of WIT and WITX are duplicated? Would it be possible for one to delegate to the other?

They are duplicated in essence but not all the details are the same; e.g., anywhere we need a slice in witx.rs we do the Wiggle GuestSlice dance. I'll look into this a bit more...

@abrown
Copy link
Member Author

abrown commented Aug 10, 2023

@alexcrichton: one more thought about your comments is that perhaps some of these improvements could be done as a separate PR. There is clearly more work that needs to be done here (e.g., replace all these temporary WIT files with their upstream versions, implement named models, etc.) so perhaps some of the refactorings you suggest could fit into some of those PRs.

@elliottt elliottt removed the request for review from a team August 10, 2023 23:55
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

Adding the representation that is a duplicate of the wasmtime-wit-bindgen generated types creates a lot of noise in this implementation. Wasmtime is going to have the component-model feature enabled in its use in the cli as soon as #6836 lands (should be very soon, just some minor conflicts), at which point you can make wasi-nn available to components as well.

I'd rather see this PR land with just the wit-bindgen generated types instead of the duplicates, rather than iterate on that later.

This removes the crate-specific types in order to use the WIT-generated
types throughout the crate. The main effect of this is that the crate
no longer optionally includes `wasmtime` with the `component-model`
feature--now that is required.
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

This looks a lot better, thanks!

.as_slice()?
.expect("cannot use with shared memories; see https://github.com/bytecodealliance/wasmtime/issues/5235 (TODO)");
let xml = &builders[0];
let weights = &builders[1];
Copy link
Contributor

@pchickey pchickey Aug 16, 2023

Choose a reason for hiding this comment

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

if the goal of this list of lists is just to pass in a list<u8> for xml and a list<u8> for weights, then can we just make it two arguments that are each list<u8>?

Copy link
Member Author

@abrown abrown Aug 16, 2023

Choose a reason for hiding this comment

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

well, other backends will have different numbers of list<u8> to be passed in so the spec has to account for that; the Backend trait mirrors that so that other backends can be implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think eventually we should use named parameters here as well, but @abrown is correct that different frameworks take a different number of options and some even take an execution plan for a model.

@abrown abrown added this pull request to the merge queue Aug 16, 2023
Merged via the queue into bytecodealliance:main with commit 6130395 Aug 16, 2023
@abrown abrown deleted the wasi-nn-in-wit branch August 16, 2023 20:09
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Aug 18, 2023
* wasi-nn: refactor to allow `preview2` access

This change refactors the `wasmtime-wasi-nn` crate to allow access from
both `preview1` and `preview2` ABIs. Though the `wasi-nn` specification
has included a WIT description for some time, here we use some in-tree
files until WebAssembly/wasi-nn#38 is landed.
The `preview2` code is not exercised anywhere yet: ideally this would be
wired up once component model `resource`s are fully implemented in
Wasmtime.

prtest:full

* wasi-nn: use `preview1` linkage

prtest:full

* review: rename `preview*` to `wit*`

This is based on @pchickey's [comments] on ABI naming.

[comments]: https://bytecodealliance.zulipchat.com/#narrow/stream/266558-wasi-nn/topic/wasi-nn.20.2B.20preview2/near/383368292

* review: update README

* fix: remove broken doc links

* fix: replace typo `wit` with `gen`

* review: use `wit` types everywhere

This removes the crate-specific types in order to use the WIT-generated
types throughout the crate. The main effect of this is that the crate
no longer optionally includes `wasmtime` with the `component-model`
feature--now that is required.

* review: move `BackendKind` conversion into `witx.rs`

* review: remove `<'a>`

* review: use `tracing` crate instead of `eprintln!`
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.

4 participants