wasmtime / PR #3977 Adding the TensorFlow backend to wasi-nn · git-wasmtime

Can this cause reading outside of dirs that the wasm module is given access to using a path like ../../../etc/passwd? Wouldn't it be better to provide the model as a byte array just like for the openvino backend?

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

It looks like only the first builder array is used below?

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

            op_x,
            op_output,

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

This doesn't look right: input_1 and Predictions seem like names for nodes in a specific model. This needs to work for any model, using set_input and set_output to pass in the tensor data.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

            backend.load(builders, target, &self.map_dir)?;

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

        if let Some(dir) = map_dir {
I think this would mean that unmap could go away below.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

This is limited to a single input and likely shouldn't be performed here. Perhaps SessionRunArgs should be stored on the context and add_feed called in set_input?

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

This looks like it should happen in set_output.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

        let mut args = SessionRunArgs::new();
Here and elsewhere: it seems like there is some extra code in this file that could be removed. Maybe try running cargo clippy on this file to see what it advises?

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

Unused... remove?

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

To interface with the wasi-nn API, we must have an ordered set of outputs for the user to be able to index into. I see that SignatureDef::outputs() is available, but this does not provide a reliable order. In any case, index should be used here to retrieve one of the possibly-multiple outputs.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

Is this a constant for all models?

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

The entire wasi-nn module can be turned on and off; @brianjjones has worked on https://github.com/tensorflow/rust/pull/354 to make sure this code path should work regardless of whether TF libraries are present on the system.

My concern here is that, until the tensorflow crate releases a new version with that PR's changes, we should fix this dependency to a specific commit to avoid any upstream issues.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

This seems incorrect: not all output tensors will be of f32 type.

Wasmtime GitHub notifications bot (Jun 02 2022 at 22:00):

abrown created PR review comment:

The saved model API does not appear to provide a way to use bytes; ideally it would, though, so maybe it is worthwhile opening an issue in the tensorflow repository.

But @bjorn3's comment is a good one in that it highlights whether this is indeed correct. It doesn't seem so: a) shouldn't a user be allowed to --map-dirs guest/foo:host/bar and then ask wasi-nn to load the model at directory guest/foo/my/model (translated to host/bar/my/model? b) if this is the case, then this code will need to learn how to prevent escapes like @bjorn3 mentions, e.g., ../../../etc/password.

I would propose that map_dirs be converted from Vec<(String, String)> to Vec<MappedDir> so that MappedDir can be the one place to hold this logic.

Wasmtime GitHub notifications bot (Jun 24 2022 at 17:14):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jun 30 2022 at 23:29):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jun 30 2022 at 23:34):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jul 12 2022 at 21:20):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jul 12 2022 at 22:12):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jul 19 2022 at 23:19):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jul 27 2022 at 19:56):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jul 27 2022 at 20:04):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 23:39):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 23:42):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:53):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:53):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:53):

abrown created PR review comment:

This should probably be a version released on crates.io.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:53):

abrown created PR review comment:

What I mean is that we should check if builders.len() != 1...

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:53):

abrown created PR review comment:

        self.map_dir = paths.clone();
I think it should just work to clone the entire vector. And we might as well just switch to using Vec as the struct type since one can detect the presence of mapped directories with is_empty(); why re-wrap in an Option?

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:53):

abrown created PR review comment:

This seems to be information injected when the model was saved; I don't think it will always be "serve"?

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:53):

abrown created PR review comment:

I'm not sure I understand why...

Wasmtime GitHub notifications bot (Aug 08 2022 at 17:12):

abrown updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Aug 08 2022 at 17:28):

abrown updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Aug 16 2022 at 20:56):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Aug 16 2022 at 21:51):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 19:58):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Aug 24 2022 at 16:22):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Aug 30 2022 at 23:00):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 01 2022 at 18:36):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 01 2022 at 19:18):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 01 2022 at 20:50):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 01 2022 at 21:07):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 10 2022 at 01:13):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

We should run the TF example here as well.

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

Can this train image be reliably retrieved from the Internet? If so, that might be better than including it in tree (below).

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

Looks like some updates are needed once a new version of the bindings crate is published... which is needed for CI to pass as well, I think.

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

Isn't there already a crate to do this?

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

We don't need to know about OpenVINO here and I think the --features wasi-nn is enabled by default so it is probably not necessary.

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

It's unclear to me why we can't just fetch the output as a &[u8] and then copy_from_slice down below. This code does two copies!

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

It's unclear to me why copy_from_slice has been replaced by all of these match cases?

Wasmtime GitHub notifications bot (Sep 15 2022 at 17:18):

abrown created PR review comment:

I think this custom comma-delimited parsing should be removed like we talked about.

Wasmtime GitHub notifications bot (Sep 19 2022 at 23:47):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 22 2022 at 00:17):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 22 2022 at 00:20):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 22 2022 at 00:33):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 22 2022 at 17:22):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 22 2022 at 19:45):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Sep 23 2022 at 16:59):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 21:01):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 01:27):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 16:54):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 17:02):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 18:12):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 18:26):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 18:27):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 21:16):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 23:12):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 02 2022 at 23:31):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Wasmtime GitHub notifications bot (Dec 03 2022 at 01:14):

brianjjones updated PR #3977 from tf_runtime_linked to main.

Last updated: Nov 22 2024 at 16:03 UTC