Stream: git-wasmtime

Topic: wasmtime / PR #6895 wasi-nn: switch testing infrastructur...

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

abrown opened PR #6895 from abrown:switch-tests to bytecodealliance:main:

Previously, all wasi-nn testing was performed by a ci/run-wasi-nn-example.sh Bash script. This was inadequate for testing the wasmtime-wasi-nn crate. This change attempts to remedy that by (1) switching the "check the environment" logic of that script over to Rust and (2) writing some more tests. The tests all perform the same MobileNet inference but at different levels of abstraction; I would prefer to test other scenarios but this involves even more infrastructure and seems to be more the responsibility of the ML backend in question. At least this change makes it easier to test more ML backends as they are added and for developers to run tests directly with cargo test. Note that part of this infrastructure code is a way to skip tests when the environment is not ready for wasi-nn; developers should not see cargo test failures if they are not interested in the wasmtime-wasi-nn crate.

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

abrown requested pchickey for a review on PR #6895.

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

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

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

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

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

abrown updated PR #6895.

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

alexcrichton created PR review comment:

I think it's ok to remove this as it's no longer in the repo

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

alexcrichton submitted PR review:

Seems reasonable to me!

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

alexcrichton submitted PR review:

Seems reasonable to me!

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

alexcrichton created PR review comment:

To avoid pulling in this dependency, could this perhaps assume that curl is an executable on the system and use Command to spawn curl to download things? (if it's only used for testing anyway in theory that should be ok)

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

alexcrichton created PR review comment:

Is the test-check feature here primarily for the reqwest dependency? If that goes away could this part go away to and unconditionally use cfg(test) for the test_check stuff?

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

abrown submitted PR review.

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

abrown created PR review comment:

Yeah, that's a better idea; I'll switch to that.

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

abrown submitted PR review.

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

abrown created PR review comment:

Well, kind of. I need to be able to use the test_check module from various places: unit tests, integration tests, Wasmtime CLI tests... When I was trying to make this work I observed that I could #[cfg(test)] the test_check module and still use it for unit tests but not for the other cases. I actually need this logic available (somehow) in the public interface of the crate. Or perhaps not "_the_ crate" but rather "_a_ crate": I could move this logic to a separate crate like wasmtime-wasi-nn-test and create a dev-dependency on that?

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

abrown updated PR #6895.

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

abrown updated PR #6895.

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

abrown updated PR #6895.

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

abrown updated PR #6895.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 14:24):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 14:24):

alexcrichton created PR review comment:

Ah ok yeah makes sense, and yes if it's required outside this crate then #[cfg(test)] won't work. That being said though since there's no dependencies behind this implementation any more with reqwest replaced I think it'd be fine to unconditionally include this functionality in the crate and other embedders would largely just ignore it.

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

abrown updated PR #6895.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2023 at 22:56):

abrown updated PR #6895.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 11 2023 at 23:25):

abrown updated PR #6895.

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

abrown commented on PR #6895:

This has been superseded by #7679.

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

abrown closed without merge PR #6895.

Last updated: Dec 23 2024 at 13:07 UTC