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.

Sign up for GitHub

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

Jump to bottom

[js/web] JSEP Gather OP #16855

Merged
merged 13 commits into from
Aug 3, 2023
Merged

[js/web] JSEP Gather OP #16855

merged 13 commits into from
Aug 3, 2023

Conversation

dakenf
Copy link
Contributor

@dakenf dakenf commented Jul 25, 2023

Description

Added Gather op that works with both i32 and i64 indices, assuming that values fall into i32 limit. The assumption is safe because it's not possible to allocate more than 2gb buffer for inputs.

It treats all data from input tensor as u32, copying 1 or 2 elements for i64, u64 and double.

@dakenf dakenf changed the title [web/js] JSEP Gather OP [js/web] JSEP Gather OP Jul 25, 2023
@dakenf
Copy link
Contributor Author

dakenf commented Jul 26, 2023

FYI: it passes all tests but does not work correctly with StableDiffusion text encoder. Will update the PR once i fix it

@satyajandhyala satyajandhyala added the ep:WebGPU ort-web webgpu provider label Jul 26, 2023
@jchen351
Copy link
Contributor

Can you run npx ts-node ./generate-webgpu-operator-md.ts from onnxruntime/js/web/script

To update the document?

@jchen351 jchen351 mentioned this pull request Jul 27, 2023
Closed
@guschmue
Copy link
Contributor

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@guschmue
Copy link
Contributor

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 16855 in repo microsoft/onnxruntime

@guschmue
Copy link
Contributor

the ci pipeline is nagging:

You can reproduce these results locally by using `lintrunner`. To set up lintrunner locally, see https://github.com/microsoft/onnxruntime/blob/main/docs/Coding_Conventions_and_Standards.md#linting . Error: Process completed with exit code 1.

@guschmue
Copy link
Contributor

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 16855 in repo microsoft/onnxruntime

@guschmue
Copy link
Contributor

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Aug 1, 2023

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Aug 1, 2023

sorry, lint still not happy.
You can repro locally in the js/ directory with
npm run lint
or fix formatting with
npm run format.

@dakenf
Copy link
Contributor Author

dakenf commented Aug 1, 2023

sorry, lint still not happy.
You can repro locally in the js/ directory with
npm run lint
or fix formatting with
npm run format.

First one did not help but the last seem to fix it

@guschmue
Copy link
Contributor

guschmue commented Aug 2, 2023

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Aug 2, 2023

/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

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 16855 in repo microsoft/onnxruntime

@guschmue
Copy link
Contributor

guschmue commented Aug 2, 2023

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Aug 2, 2023

/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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Aug 2, 2023

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 16855 in repo microsoft/onnxruntime

@guschmue
Copy link
Contributor

guschmue commented Aug 2, 2023

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 16855 in repo microsoft/onnxruntime

@guschmue
Copy link
Contributor

guschmue commented Aug 3, 2023

/azp run ONNX Runtime Web CI Pipeline

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Aug 3, 2023

/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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 9 pipeline(s).

@guschmue
Copy link
Contributor

guschmue commented Aug 3, 2023

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

@guschmue guschmue requested a review from satyajandhyala August 3, 2023 16:13
@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

guschmue
guschmue approved these changes Aug 3, 2023
View reviewed changes
js/web/lib/wasm/jsep/webgpu/ops/gather.ts Outdated Show resolved Hide resolved
@guschmue guschmue requested a review from satyajandhyala August 3, 2023 20:04
@satyajandhyala satyajandhyala merged commit ea55700 into microsoft:main Aug 3, 2023
@dakenf
Copy link
Contributor Author

dakenf commented Aug 9, 2023 •
edited
Loading

You should know that there is some kind of issue with this OP, because when I run it with StableDiffusion, it throws an error MatMul dimensions do not match. Before recent pull from main branch it was throwing another error (with Reshape OP)
If i comment out these:

using AllSupportedSize =
    TypeList<
        float,
//        double,
//        int64_t,
//        uint64_t,
        int32_t,
        uint32_t>;

everything works fine. And also MemCopy (from and to host) ops double when code is not commented out. From ~1500 to ~3500. I guess that's because all binary and unary ops don't support int64
I cannot pinpoint the error yet because i'm running a patched runtime for 64bit and a patched browser to support 64bit multi-threading, so it might be because i've messed up somewhere. But with those lines commented or when using CPU provider it works fine

Do you know an easy way to output every node output to browser console so i can compare and find the issue faster?

jchen351 pushed a commit that referenced this pull request Aug 12, 2023
@dakenf @guschmue @jchen351
[js/web] JSEP Gather OP (#16855)
be80a2e 
Added Gather op that works with both i32 and i64 indices, assuming that
values fall into i32 limit. The assumption is safe because it's not
possible to allocate more than 2gb buffer for inputs.

It treats all data from input tensor as u32, copying 1 or 2 elements for
i64, u64 and double.

---------

Co-authored-by: Guenther Schmuelling <[email protected]>
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
@dakenf @guschmue @kleiti
[js/web] JSEP Gather OP (microsoft#16855)
edd6b07 
### Description
Added Gather op that works with both i32 and i64 indices, assuming that
values fall into i32 limit. The assumption is safe because it's not
possible to allocate more than 2gb buffer for inputs.

It treats all data from input tensor as u32, copying 1 or 2 elements for
i64, u64 and double.

---------

Co-authored-by: Guenther Schmuelling <[email protected]>
siweic0 pushed a commit to siweic0/onnxruntime-web that referenced this pull request May 9, 2024
@dakenf @guschmue
[js/web] JSEP Gather OP (microsoft#16855)
9c14b03 
### Description
Added Gather op that works with both i32 and i64 indices, assuming that
values fall into i32 limit. The assumption is safe because it's not
possible to allocate more than 2gb buffer for inputs.

It treats all data from input tensor as u32, copying 1 or 2 elements for
i64, u64 and double.

---------

Co-authored-by: Guenther Schmuelling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guschmue guschmue guschmue approved these changes

@satyajandhyala satyajandhyala satyajandhyala approved these changes

Assignees
No one assigned
Labels
ep:WebGPU ort-web webgpu provider
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dakenf @jchen351 @guschmue @satyajandhyala