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

Support loading models by name #38

Merged
merged 15 commits into from
Aug 9, 2023

Conversation

geekbeast
Copy link
Contributor

@geekbeast geekbeast commented Apr 3, 2023

This change has been scoped down to only adding support for getting a handle to a named model for inference and updating the WIT bindings to the latest spec.

wasi-nn.witx Outdated Show resolved Hide resolved
wasi-nn.witx Outdated Show resolved Hide resolved
wasi-nn.witx Outdated Show resolved Hide resolved
wasi-nn.witx Outdated Show resolved Hide resolved
wasi-nn.witx Outdated Show resolved Hide resolved
wasi-nn.witx Outdated Show resolved Hide resolved
wasi-nn.witx Outdated Show resolved Hide resolved
@mingqiusun
Copy link
Collaborator

I think the register/deregister methods make sense for the FaaS scenario, but optional/unnecessary for a standalone env. We should document those two use cases, maybe via sample programs?

@geekbeast geekbeast requested a review from pchickey August 7, 2023 19:02
@geekbeast geekbeast changed the title Feature/named models Support loading models by name Aug 7, 2023
@geekbeast geekbeast requested a review from abrown August 7, 2023 19:16
wit/wasi-nn.wit Outdated
import execution
import errors

export run: func()-> result<_,string>
Copy link
Contributor

Choose a reason for hiding this comment

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

A world definition that exports a run like this is meant for a program that imports those four interfaces, takes no arguments, and has no other imports. I don't think that is actually a world anyone wants to target.

My expectation is that downstream of this spec, embedders will provide the wasi-nn imports, but the export funcs will be part of some other spec. e.g. if its a CLI program the export will be provided by the wasi:cli/command world, and if its used in a web server the export will be provided by the wasi:http/proxy world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may have been a stray experimentation error while I was trying to track down what was causing the wit-bindgen failure. This can be removed entirely.

That said, I wasn't 100% clear on what is going to get exported here for use by other components. Do the graph and execution interfaces need to be exported so that other components can perform inference?

Copy link
Contributor

Choose a reason for hiding this comment

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

a world is from the perspective of the user, as opposed to an implementer: import in the world definition says, if I'm writing a component that uses inference I have these interfaces available to import. the implementer of a world needs to make imports available as exports.

wit/wasi-nn.wit Outdated Show resolved Hide resolved
wit/wasi-nn.wit Outdated Show resolved Hide resolved
wit/wasi-nn.wit Outdated Show resolved Hide resolved
wit/wasi-nn.wit Show resolved Hide resolved
wit/wasi-nn.wit Show resolved Hide resolved
wasi-nn.witx Show resolved Hide resolved
wit/wasi-nn.wit Show resolved Hide resolved
README.md Outdated
@@ -110,7 +110,7 @@ used to solve the given problem.

### Detailed design discussion

For the details of the API, see [wasi-nn.wit.md](wasi-nn.wit.md).
For the details of the API, see [wasi-nn.wit.md](legacy_wit/wasi-nn.wit.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear to me why this file was renamed and why the pointer is to it instead of to wit/wasi-nn.wit

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 will just delete it and point it at the new file. If someone needs access they can pull from git history.

@geekbeast geekbeast requested a review from pchickey August 8, 2023 07:04
@abrown abrown mentioned this pull request Aug 8, 2023
wasi-nn.witx Outdated Show resolved Hide resolved
wasi-nn.witx Outdated Show resolved Hide resolved
wit/wasi-nn.wit Show resolved Hide resolved
wit/wasi-nn.wit Show resolved Hide resolved
wit/wasi-nn.wit Show resolved Hide resolved
wit/wasi-nn.wit Show resolved Hide resolved
abrown added a commit to abrown/wasmtime that referenced this pull request 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 `resource`s are fully implemented in
Wasmtime.
abrown added a commit to abrown/wasmtime that referenced this pull request 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 `resource`s are fully implemented in
Wasmtime.
abrown added a commit to abrown/wasmtime that referenced this pull request 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 `resource`s are fully implemented in
Wasmtime.
abrown added a commit to abrown/wasmtime that referenced this pull request 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 `resource`s are fully implemented in
Wasmtime.

prtest:full
abrown added a commit to abrown/wasmtime that referenced this pull request Aug 9, 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 `resource`s are fully implemented in
Wasmtime.

prtest:full
Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

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

Ok, let's merge this. This is enough to implement "named models" and we can iterate on these definitions as we go along. (And I guess I'll fix the failing CI in another PR).

@abrown abrown merged commit c1ff124 into WebAssembly:main Aug 9, 2023
1 check failed
abrown added a commit to abrown/wasi-nn-spec that referenced this pull request Aug 9, 2023
abrown added a commit that referenced this pull request Aug 9, 2023
abrown added a commit to abrown/wasi-nn-spec that referenced this pull request Aug 14, 2023
This change also removes some extra functions that snuck through the
review of WebAssembly#38: `set-input-by-name`, `eval`.
abrown added a commit that referenced this pull request Aug 14, 2023
* Move original documentation to new `wasi-nn.wit` file

This change also removes some extra functions that snuck through the
review of #38: `set-input-by-name`, `eval`.

* Move `wasi-nn.wit` to top-level directory

* Remove original `*.wit.md` documents

* Tweak `get-output` return type
github-merge-queue bot pushed a commit to bytecodealliance/wasmtime that referenced this pull request Aug 16, 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!`
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