Stream: git-wasmtime

Topic: wasmtime / PR #6854 wasi-nn: add named models


view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):

abrown opened PR #6854 from abrown:registries to bytecodealliance:main:

This implements named models in Wasmtime; see the commit messages for more details.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):

abrown requested fitzgen for a review on PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):

abrown requested wasmtime-default-reviewers for a review on PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):

abrown requested wasmtime-core-reviewers for a review on PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):

abrown requested pchickey for a review on PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 00:56):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 00:56):

abrown created PR review comment:

I'm not quite happy with using this script for testing. It was good initially to get things working but I would prefer to be using cargo test instead. The problem is that there is a dependency expectation: this can only run if the system has OpenVINO installed, which will not always be the case for developers running cargo test.

I can think of two workarounds:

Interested in some feedback on whether one of those approaches is better than this script!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 16:36):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 16:36):

abrown created PR review comment:

Another thing I'm not quite sure of is whether this should return Option<&mut Graph>, Option<&Graph>, or even Option<Graph> (seeing as how Graph is just an Arc-wrapper). We immediately clone the graph and I removed the mutability requirement for BackendGraph::init_execution_context... @geekbeast , any opinions from implementing other kinds of GraphRegistry?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2023 at 16:36):

abrown edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 20:57):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 20:57):

alexcrichton created PR review comment:

Personally I like your second suggestion and matches what I was going to suggest as well. We do run a higher risk of not actually running the tests in CI but I think running something by default as part of cargo test --workspace is a good mitigating factor for that.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2023 at 01:40):

geekbeast submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 20 2023 at 01:40):

geekbeast created PR review comment:

If the tests are for wasi-nn, we should just have a test backend that does something basic like an affine layer.

If this is testing integration we should think about how we handle multiple backends. This same issue will come up with pytorch, onnxruntime, tf, etc.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 16:46):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 16:46):

abrown created PR review comment:

@alexcrichton, I have a whole other set of commits ready for switching to that kind of testing. Just waiting on a review of this one because the new testing also covers the load_by_name logic added here.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 19:17):

alexcrichton submitted PR review:

I can't review the wasi-nn stuff specifically really, it all seems reasonable though. The src/commands/run.rs bits look good to me, although I might recommend renaming --graph to something like --wasi-nn-graph or --nn-graph or something like that perhaps to make it a bit more clear it's for wasi-nn as opposed to something else graph-related (not that we have much else like that right now)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 23:15):

abrown updated PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 23:26):

abrown updated PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 23:46):

abrown has enabled auto merge for PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 00:55):

abrown merged PR #6854.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 22:40):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 22:40):

abrown created PR review comment:

Ok, check out https://github.com/bytecodealliance/wasmtime/pull/6895 if you're interested in where I went with this.


Last updated: Dec 23 2024 at 12:05 UTC