Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a higher-level public API for Rust users #68

Open
abrown opened this issue Aug 31, 2022 · 4 comments
Open

Provide a higher-level public API for Rust users #68

abrown opened this issue Aug 31, 2022 · 4 comments

Comments

@abrown
Copy link
Collaborator

abrown commented Aug 31, 2022

The functions defined in wasi_nn::wasi_ephemeral_nn::* are the raw Wasm symbols used to link with the host's implementation of wasi-nn. The wasi-nn Rust bindings should not expose these, since they should be providing a higher level of abstraction. These symbols are visible in the documentation here.

@abrown
Copy link
Collaborator Author

abrown commented Oct 28, 2022

The following is a sketch of what the higher-level API could look like:

struct Graph { ... }
impl Graph {
  pub fn load(data: impl Iterator<Item = &[u8]>, encoding: GraphEncoding, target: ExecutionTarget) -> Result<Self>;
  pub fn get_execution_context(&self) -> Result<ExecutionContext>;
  pub fn get_input_types(&self) -> TensorTypes;
  pub fn get_output_types(&self) -> TensorTypes
}

struct TensorTypes { ... }
impl TensorTypes {
  pub fn len(&self) -> usize;
  pub fn get(&self, index: u32) -> Result<TensorType>;
}

struct ExecutionContext { ... }
impl ExecutionContext {
  pub fn set_input(&mut self, index: u32, tensor: Tensor) -> Result<()>;
  pub fn compute(&mut self) -> Result<()>;
  pub fn get_output(&self, index: u32, index: u32) -> Result<Tensor>;
}

Some considerations:

  • perhaps get_execution_context must be a &mut self, but it would be preferable if it were not
  • ExecutionContext may need a lifetime parameter since a Tensor may have an associated lifetime
  • we should not make get_output consume self since there may be multiple outputs that need to be retrieved
  • note how none of the APIs need be unsafe
  • to support introspecting the graph inputs and outputs, the wasi-nn spec will need some additions (e.g., get_input_types_len, get_input_type, get_output_types_len, get_output_type)

@abrown abrown changed the title Wasm-level symbols should not be exposed in the public API Provide a higher-level public API for Rust users Oct 28, 2022
@abrown
Copy link
Collaborator Author

abrown commented Oct 28, 2022

The functions defined in wasi_nn::wasi_ephemeral_nn::* are the raw Wasm symbols used to link with the host's implementation of wasi-nn. The wasi-nn Rust bindings should not expose these, since they should be providing a higher level of abstraction. These symbols are visible in the documentation here.

Once new bindings are created, the old low-level symbols should be removed from the public API.

@geekbeast
Copy link
Contributor

We should also address #13 as well. Being able to generate a compatible tensor server side seems like it will be the entry point to most workflows.

My initial thoughts are that wasi-nn shouldn't care about correctly implementing the mapping from images onto tensors, but we may want to provide a conversion api for bf16 if it is commonly used. I see no reason we can't provide helper functions for extracting tensor from the image, but they shouldn't be part of the spec.

@abrown
Copy link
Collaborator Author

abrown commented Nov 29, 2022

We should also address #13 as well.

@geekbeast, commented over in that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants