Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50): 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.
Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):abrown requested fitzgen for a review on PR #6854.
Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):abrown requested wasmtime-default-reviewers for a review on PR #6854.
Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):abrown requested wasmtime-core-reviewers for a review on PR #6854.
Wasmtime GitHub notifications bot (Aug 17 2023 at 00:50):abrown requested pchickey for a review on PR #6854.
Wasmtime GitHub notifications bot (Aug 17 2023 at 00:56):abrown submitted PR review.
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 testinstead. 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 runningcargo test.I can think of two workarounds:
#[ignore]the OpenVINO-specific tests and add some comments telling developers to turn them on to fully test the system- add some checks at the beginning of each test to early-exit if OpenVINO is not available; the issue with this is that users may see "false success" if something goes wrong in the environment, so perhaps a
FORCE_RUN_OPENVINO_TESTSenv variable is necessary?Interested in some feedback on whether one of those approaches is better than this script!
Wasmtime GitHub notifications bot (Aug 17 2023 at 16:36):abrown submitted PR review.
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 evenOption<Graph>(seeing as howGraphis just anArc-wrapper). We immediately clone the graph and I removed the mutability requirement forBackendGraph::init_execution_context... @geekbeast , any opinions from implementing other kinds ofGraphRegistry?
Wasmtime GitHub notifications bot (Aug 17 2023 at 16:36):abrown edited PR review comment.
Wasmtime GitHub notifications bot (Aug 18 2023 at 20:57):alexcrichton submitted PR review.
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 --workspaceis a good mitigating factor for that.
Wasmtime GitHub notifications bot (Aug 20 2023 at 01:40):geekbeast submitted PR review.
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.
Wasmtime GitHub notifications bot (Aug 22 2023 at 16:46):abrown submitted PR review.
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_namelogic added here.
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.rsbits look good to me, although I might recommend renaming--graphto something like--wasi-nn-graphor--nn-graphor 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)
Wasmtime GitHub notifications bot (Aug 22 2023 at 23:15):abrown updated PR #6854.
Wasmtime GitHub notifications bot (Aug 22 2023 at 23:26):abrown updated PR #6854.
Wasmtime GitHub notifications bot (Aug 22 2023 at 23:46):abrown has enabled auto merge for PR #6854.
Wasmtime GitHub notifications bot (Aug 23 2023 at 00:55):abrown merged PR #6854.
Wasmtime GitHub notifications bot (Aug 23 2023 at 22:40):abrown submitted PR review.
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: Nov 22 2024 at 16:03 UTC