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

[js/webnn] Enable user-supplied MLContext #20600

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

egalli
Copy link
Contributor

@egalli egalli commented May 7, 2024

Description

This PR enables the API added in #20816 as well as moving context creation to JS.

Motivation and Context

In order to enable I/O Binding with the upcoming MLBuffer API in the WebNN specification, we need to share the same MLContext across multiple sessions. This is because MLBuffers are restricted to the MLContext where they were created. This PR enables developers to use the same MLContext across multiple sessions.

Copy link
Contributor

@Honry Honry left a comment

Choose a reason for hiding this comment

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

Thank you @Galli, very good starting for WebNN I/O binding support!

js/web/lib/wasm/jsep/backend-webnn.ts Outdated Show resolved Hide resolved
js/web/lib/wasm/jsep/backend-webnn.ts Outdated Show resolved Hide resolved
@Honry
Copy link
Contributor

Honry commented May 8, 2024

@fs-eire, @guschmue, @fdwr, PTAL, thanks!

Copy link
Contributor

@Honry Honry left a comment

Choose a reason for hiding this comment

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

Generally LGTM % a nit, I will test it and let you know if there's any other issues. :)

js/web/lib/wasm/jsep/backend-webnn.ts Outdated Show resolved Hide resolved
@fs-eire
Copy link
Contributor

fs-eire commented May 9, 2024

The current implementation has a few problems:

  • If user specify multiple execution providers (which is legit in ORT) like ['webgpu', { name: 'webnn', powerPreference: 'high-performance' }], it is hard to tell from JavaScript code that which EP is currently in use. As long as WebNN is initialized, JavaScript has no idea which EP will actually work with the current model - and by reading from execution providers config is not sufficient to tell.

  • The current implementation splited the code of initialization process into multiple places. The C++ code does a few things and the Javascript does some others. The [webnn session option] to [context ID] is a 1:1 mapping, and the [session ID] to [context ID] is a multi-to-one mapping. In my understanding, using a singleton map in C++ should be a better way to implement this requirement. This is because it's much easier to read and understand and putting all related code together if possible to reduce the chance to introduce bugs in future changes. Please let me know if I understand this part wrong.

  • Modifying the user input (session options) is a concern. This is usually not an expected behavior however the current implementation depends on adding a property to a user specified session options.

@egalli egalli force-pushed the move_ml_context branch from 064865e to 81695d1 Compare May 10, 2024 18:20
@egalli egalli changed the title [WebNN EP] Move MLContext creation to TypeScript [WebNN EP] Move MLContext creation to a singleton May 10, 2024
@egalli
Copy link
Contributor Author

egalli commented May 10, 2024

I have moved the context de-duplication to a singleton in C++.

@fs-eire
Copy link
Contributor

fs-eire commented May 20, 2024

Add a few comments here:

There is a new issue (#20729) reveals a clearer picture of how an actual requirement would be. Users may want to manipulate with the MLContext with more flexibility. I am currently thinking about it may be a good idea to let user to create the MLContext and just pass it to ORT via session options.

Considering the latest spec: https://www.w3.org/TR/webnn/#api-ml-createcontext

There will be a webnn-webgpu interop and createContext() may accept an WebGPU gpuDevice object. This will be even more difficult to implement inside ORT so just let users to do their part.

@egalli
Copy link
Contributor Author

egalli commented Jun 3, 2024

@fs-eire given that the API changes have landed ( #20816 ) should I modify this PR to use the new API directly or should I convert the code back to JS?

@fs-eire
Copy link
Contributor

fs-eire commented Jun 4, 2024

@fs-eire given that the API changes have landed ( #20816 ) should I modify this PR to use the new API directly or should I convert the code back to JS?

Since the API may accept MLContext as user input, we anyway need to pass MLContext from JS to C++. So it may be a good idea to create and manage MLContext in JS.

This change enables the API added in microsoft#20816 as well as moving context creation
to JS.
@egalli egalli force-pushed the move_ml_context branch from 81695d1 to 8bf9185 Compare June 10, 2024 16:48
@egalli egalli changed the title [WebNN EP] Move MLContext creation to a singleton [js/webnn] Enable user-supplied MLContext Jun 10, 2024
@egalli
Copy link
Contributor Author

egalli commented Jun 10, 2024

@fs-eire I have updated the PR to use the new API, PTAL.

Copy link
Contributor

@Honry Honry left a comment

Choose a reason for hiding this comment

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

👍 % a nit.

js/web/lib/wasm/wasm-core-impl.ts Show resolved Hide resolved
@Honry
Copy link
Contributor

Honry commented Jun 19, 2024

@fs-eire, gently ping. :)

@guschmue
Copy link
Contributor

/azp run ONNX Runtime Web CI Pipeline,Windows GPU CI Pipeline,Linux Android Emulator QNN CI Pipeline

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@egalli
Copy link
Contributor Author

egalli commented Jun 25, 2024

@guschmue I have fixed the linting issues. That said, I'm concerned about adding // Copyright (c) Microsoft Corporation. All rights reserved. to the webnn type declaration file that will eventually be moved to webmachinelearning/webnn-types. Do you mind rerunning the CI?

@guschmue
Copy link
Contributor

guschmue commented Jul 3, 2024

/azp run ONNX Runtime Web CI Pipeline,Windows GPU CI Pipeline,Linux Android Emulator QNN CI Pipeline

@guschmue
Copy link
Contributor

guschmue commented Jul 3, 2024

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,Windows ARM64 QNN CI Pipeline,Windows CPU CI Pipeline

@guschmue
Copy link
Contributor

guschmue commented Jul 3, 2024

/azp run Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,Windows x64 QNN CI Pipeline,Big Models

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Jul 8, 2024

/azp run Windows GPU TensorRT CI Pipeline,onnxruntime-binary-size-checks-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,Windows x64 QNN CI Pipeline,Big Models

Copy link

Azure Pipelines successfully started running 7 pipeline(s).

@guschmue guschmue added the ep:WebNN WebNN execution provider label Jul 8, 2024
@guschmue guschmue merged commit 4c3c809 into microsoft:main Jul 8, 2024
73 of 74 checks passed
@egalli egalli deleted the move_ml_context branch October 22, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ep:WebNN WebNN execution provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants