Stream: git-wasmtime

Topic: wasmtime / PR #2859 wasi-nn: turn it on by default

Wasmtime GitHub notifications bot (Apr 26 2021 at 22:54):

abrown opened PR #2859 from runtime-wasi-nn to main:

This change makes the wasi-nn Cargo feature a default feature. Previously, a wasi-nn user would have to build a separate Wasmtime binary (e.g. cargo build --features wasi-nn ...) to use wasi-nn and the resulting binary would require OpenVINO shared libraries to be present in the environment in order to run (otherwise it would fail immediately with linking errors). With recent changes to the openvino crate, the wasi-nn implementation can defer the loading of the OpenVINO shared libraries until runtime (i.e., when the user Wasm program calls wasi_ephemeral_nn::load) and display a user-level error if anything goes wrong (e.g., the OpenVINO libraries are not present on the system). This runtime-linking addition allows the wasi-nn feature to be turned on by default and shipped with upcoming releases of Wasmtime. This change should be transparent for users who do not use wasi-nn: the openvino crate is small and the newly-available wasi-nn imports only affect programs in which they are used.

For those interested in reviewing the runtime linking approach added to the openvino crate, see https://github.com/intel/openvino-rs/pull/19.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 26 2021 at 22:54):

abrown requested jedisct1 for a review on PR #2859.

Wasmtime GitHub notifications bot (Apr 26 2021 at 22:54):

abrown requested alexcrichton and jedisct1 for a review on PR #2859.

Wasmtime GitHub notifications bot (Apr 26 2021 at 22:54):

abrown requested alexcrichton, jedisct1 and sunfishcode for a review on PR #2859.

Wasmtime GitHub notifications bot (Apr 26 2021 at 22:55):

abrown requested tschneidereit, alexcrichton, jedisct1 and sunfishcode for a review on PR #2859.

Wasmtime GitHub notifications bot (Apr 26 2021 at 23:07):

abrown requested tschneidereit, alexcrichton, pchickey, jedisct1 and sunfishcode for a review on PR #2859.

Wasmtime GitHub notifications bot (Apr 28 2021 at 15:50):

abrown updated PR #2859 from runtime-wasi-nn to main.

Wasmtime GitHub notifications bot (Apr 28 2021 at 17:13):

abrown updated PR #2859 from runtime-wasi-nn to main.

Wasmtime GitHub notifications bot (Apr 28 2021 at 17:39):

abrown updated PR #2859 from runtime-wasi-nn to main.

Wasmtime GitHub notifications bot (Apr 29 2021 at 11:06):

tschneidereit submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 11:06):

tschneidereit submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 11:06):

tschneidereit created PR Review Comment:

Hmm, I'm not sure how I feel about the drift between API and command line parameters here. I worry there might be some risk of someone learning the CLI parameters and then assuming WasiModules::all() means the same as --wasi-modules=all.

Wasmtime GitHub notifications bot (Apr 29 2021 at 14:58):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 14:58):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 14:58):

alexcrichton created PR Review Comment:

I don't think this needs to be in the wasmtime crate since I don't think it's read by anything in the wasmtime crate?

Wasmtime GitHub notifications bot (Apr 29 2021 at 14:58):

alexcrichton created PR Review Comment:

Instead of being an assert could this return a more first-class error to avoid panicking?

Wasmtime GitHub notifications bot (Apr 29 2021 at 14:58):

alexcrichton created PR Review Comment:

I think it's ok to avoid matching -all-experimental here since -all suffices for this use case.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:10):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:10):

abrown created PR Review Comment:

Yup, moved to src/lib.rs.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:10):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:10):

abrown created PR Review Comment:

After discussing today, this now only matches default and -default.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:12):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:12):

abrown created PR Review Comment:

WasiModules::all() is now gone; "default" corresponds to WasiModules::default() and "-default" corresponds to WasiModules::none().

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:16):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:16):

abrown created PR Review Comment:

Ok, I went with bail!? I think some time down the road this type of check should be able to go away if we were to remove the special build flags for these WASI modules.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:31):

abrown updated PR #2859 from runtime-wasi-nn to main.

Wasmtime GitHub notifications bot (Apr 29 2021 at 17:34):

abrown requested alexcrichton, pchickey, jedisct1 and sunfishcode for a review on PR #2859.

Wasmtime GitHub notifications bot (Apr 29 2021 at 18:02):

abrown updated PR #2859 from runtime-wasi-nn to main.

Wasmtime GitHub notifications bot (Apr 29 2021 at 18:23):

abrown updated PR #2859 from runtime-wasi-nn to main.

Wasmtime GitHub notifications bot (Apr 29 2021 at 18:25):

abrown updated PR #2859 from runtime-wasi-nn to main.

Wasmtime GitHub notifications bot (Apr 29 2021 at 20:03):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2021 at 20:03):

alexcrichton merged PR #2859.

Last updated: Nov 22 2024 at 17:03 UTC