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

[Examples] Add the named model feature into pytorch demo. #27

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

CaptainVincent
Copy link
Member

@CaptainVincent CaptainVincent commented Aug 27, 2023

This PR is pending for the wasinn plugin 0.13.4 release. Ready

Copy link
Member

juntao commented Aug 27, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Commit e54da7c2a7b1ca36e3d0bd1be52f84ef8e5d5dfc

Key changes in the patch:

  1. Added a new Rust file named named_model.rs that contains code for executing the model with a named input tensor and output tensor.
  2. Added a new binary target wasmedge-wasinn-example-mobilenet-image-named-model in the Cargo.toml file.
  3. Added instructions in the README on how to use the new named model feature with an example command.

Potential problems:

  1. The patch does not provide any tests for the added functionality. Adding tests would ensure the correctness of the implementation and make it easier for future contributors to maintain the code.
  2. The patch does not include any documentation for the newly added code. Adding comments and docstrings to explain the functionality and usage of the named model feature would improve code readability and maintainability.
  3. There is a minor typo in the README file. The path mentioned for the output WASM file should be corrected to wasmedge-wasinn-example-mobilenet-image-named-model.wasm.
  4. The patch does not handle potential errors that could occur during file operations or WASI-NN API calls. Adding error handling would make the code more robust and prevent unexpected crashes or incorrect behavior.
  5. The patch does not include any performance optimizations or improvements. Depending on the specific use case, optimizing the image processing or inference computation could be beneficial.



```bash
wasmedge --dir .:. --nn-preload demo:PyTorch:CPU:mobilenet.pt wasmedge-wasinn-example-mobilenet-image-named-model.wasm demo input.jpg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what the demo:PyTorch:CPU:mobilenet.pt name means?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is defined in the named model feature: --nn-preload
Ref: WebAssembly/wasi-nn#36

Copy link
Member Author

@CaptainVincent CaptainVincent Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this format could be determined by the runtime implementation.
I mostly followed his original proposal style <name>:<encoding>:<model path>.
But I add the necessary info for build model graph, format will follow <name>:<encoding>:<target>:<model path>.
And the name need the same with load_from_cache(<name>) inside .rs

I will update README.md later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the encoding and target are determined by the API calls in the code. What if the information in the name conflicts with the settings in the code? Why do we have to repeat ourselves here?

Copy link
Member Author

@CaptainVincent CaptainVincent Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, wasi_ephemeral_nn::load_by_name doesn't have parameters to specify the target and encoding of building the graph. Similarly, you can't assign a name to a model through build_from_files ( or bytes) relying on wasi_ephemeral_nn::load (loading a model from WebAssembly). These two mechanisms, IIUC from the API definition, there don't have interoperability. By the way, there's a scenario where --nn-preload multiple models with conflicting names can occur (our implementation the later one will overwrite the earlier one).

@hydai hydai merged commit 3db57ce into second-state:master Sep 11, 2023
4 checks passed
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

Successfully merging this pull request may close these issues.

3 participants