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

Support Gather and don't reshape scalar to [1] #9

Merged
merged 3 commits into from
May 17, 2023
Merged

Support Gather and don't reshape scalar to [1] #9

merged 3 commits into from
May 17, 2023

Conversation

Honry
Copy link
Owner

@Honry Honry commented May 17, 2023

No description provided.

@Honry
Copy link
Owner Author

Honry commented May 17, 2023

@fdwr, PTAL, thanks!

Honry
Honry commented May 17, 2023
View reviewed changes
onnxruntime/core/providers/webnn/builders/impl/gather_op_builder.cc Outdated
const logging::Logger& logger) const {
const auto& input_defs = node.InputDefs();
std::vector<int64_t> input_shape;
if (!GetShape(*input_defs[0], input_shape, logger)) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

@fdwr, any input/options limitation should I check for Gather on WebNN?

Copy link

@fdwr fdwr May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not many, just that input.rank >= 1.

  • axis within [-input.rank, input.rank - 1] (which I suspect HandleNegativeAxis already handles)
  • input.rank >= 1
  • indices.rank == any (even 0)
  • output.rank == indices.rank + input.rank - 1

fdwr
fdwr reviewed May 17, 2023
View reviewed changes
onnxruntime/core/providers/webnn/builders/model_builder.cc Outdated
@@ -97,8 +97,8 @@ Status ModelBuilder::RegisterInitializers() {
const auto& shape = tensor.dims();
std::vector<int32_t> dims;
if (shape.empty()) {
// This is a scalar initializer, WebNN requires a shape, make this a {1} tensor.
dims = {1};
// This is a scalar initializer.
Copy link

@fdwr fdwr May 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you need the special if branch, since the general else branch with transform will do no work anyway (cend == cbegin) in that case?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Just want to inform readers that what a scalar is presented in WebNN EP. I could remove if branch and add additional comment to inform this.

github-advanced-security[bot]
github-advanced-security bot found potential problems May 17, 2023
View reviewed changes
onnxruntime/core/providers/webnn/builders/impl/gather_op_builder.cc
@@ -0,0 +1,73 @@
// Copyright (c) Microsoft Corporation. All rights reserved.

Check warning

Code scanning / lintrunner

CLANGFORMAT/format

See https://clang.llvm.org/docs/ClangFormat.html. Run `lintrunner -a` to apply this patch.
@Honry
Copy link
Owner Author

Honry commented May 17, 2023

Thanks @fdwr, comments addressed, PTAL again.

fdwr
fdwr approved these changes May 17, 2023
View reviewed changes
Copy link

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

Small comment typos. Otherwise LGTM - thanks Honry.

@Honry @fdwr
Update onnxruntime/core/providers/webnn/builders/impl/gather_op_build…
07fda9d 
…er.cc

Co-authored-by: Dwayne Robinson <fdwr@hotmail.com>
@Honry Honry merged commit 7776f6d into stable-diffusion May 17, 2023
@fdwr fdwr mentioned this pull request May 19, 2023
Closed
Honry pushed a commit that referenced this pull request Aug 28, 2023
@snnn
nodejs: Release Ort Env before main function returns (microsoft#17288)
3e93403 
### Description
Release OrtEnv before main function returns. Before this change, OrtEnv
is deleted when C/C++ runtime destructs all global variables in ONNX
Runtime's core framework.
The callstack is like this:
```
  * frame #0: 0x00007fffee39f5a6 libonnxruntime.so.1.16.0`onnxruntime::Environment::~Environment(this=0x00007fffee39fbf2) at environment.h:20:7
    frame #1: 0x00007fffee39f614 libonnxruntime.so.1.16.0`std::default_delete<onnxruntime::Environment>::operator()(this=0x00007ffff4c30e50, __ptr=0x0000000005404b00) const at unique_ptr.h:85:2
    frame #2: 0x00007fffee39edca libonnxruntime.so.1.16.0`std::unique_ptr<onnxruntime::Environment, std::default_delete<onnxruntime::Environment>>::~unique_ptr(this=0x5404b00) at unique_ptr.h:361:17
    frame #3: 0x00007fffee39e2ab libonnxruntime.so.1.16.0`OrtEnv::~OrtEnv(this=0x00007ffff4c30e50) at ort_env.cc:43:1
    frame #4: 0x00007fffee39fa96 libonnxruntime.so.1.16.0`std::default_delete<OrtEnv>::operator()(this=0x00007fffefff8f78, __ptr=0x00007ffff4c30e50) const at unique_ptr.h:85:2
    frame #5: 0x00007fffee39f394 libonnxruntime.so.1.16.0`std::unique_ptr<OrtEnv, std::default_delete<OrtEnv>>::~unique_ptr(this=0x7ffff4c30e50) at unique_ptr.h:361:17
    frame #6: 0x00007ffff78574b5 libc.so.6`__run_exit_handlers + 261
    frame #7: 0x00007ffff7857630 libc.so.6`exit + 32
    frame #8: 0x00007ffff783feb7 libc.so.6`__libc_start_call_main + 135
    frame #9: 0x00007ffff783ff60 libc.so.6`__libc_start_main@@GLIBC_2.34 + 128
    frame #10: 0x0000000000abbdee node`_start + 46
```
After this change, OrtEnv will be deleted before the main function
returns and nodejs is still alive.
Honry pushed a commit that referenced this pull request Aug 7, 2024
@mszhanyi
Fix docker image layer caching to avoid redundant docker building and…
0d1da41 
… transient connection exceptions. (microsoft#21612)

### Description
Improve docker commands to make docker image layer caching works.
It can make docker building faster and more stable.
So far, A100 pool's system disk is too small to use docker cache.
We won't use pipeline cache for docker image and remove some legacy
code.

### Motivation and Context
There are often an exception of
```
64.58 + curl https://nodejs.org/dist/v18.17.1/node-v18.17.1-linux-x64.tar.gz -sSL --retry 5 --retry-delay 30 --create-dirs -o /tmp/src/node-v18.17.1-linux-x64.tar.gz --fail
286.4 curl: (92) HTTP/2 stream 0 was not closed cleanly: INTERNAL_ERROR (err 2)
```
Because Onnxruntime pipeline have been sending too many requests to
download Nodejs in docker building.
Which is the major reason of pipeline failing now

In fact, docker image layer caching never works.
We can always see the scrips are still running
```
#9 [3/5] RUN cd /tmp/scripts && /tmp/scripts/install_centos.sh && /tmp/scripts/install_deps.sh && rm -rf /tmp/scripts
#9 0.234 /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
#9 0.235 /bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8)
#9 0.235 /tmp/scripts/install_centos.sh: line 1: !/bin/bash: No such file or directory
#9 0.235 ++ '[' '!' -f /etc/yum.repos.d/microsoft-prod.repo ']'
#9 0.236 +++ tr -dc 0-9.
#9 0.236 +++ cut -d . -f1
#9 0.238 ++ os_major_version=8
....
#9 60.41 + curl https://nodejs.org/dist/v18.17.1/node-v18.17.1-linux-x64.tar.gz -sSL --retry 5 --retry-delay 30 --create-dirs -o /tmp/src/node-v18.17.1-linux-x64.tar.gz --fail
#9 60.59 + return 0
...
```

This PR is improving the docker command to make image layer caching
work.
Thus, CI won't send so many redundant request of downloading NodeJS.
```
#9 [2/5] ADD scripts /tmp/scripts
#9 CACHED

#10 [3/5] RUN cd /tmp/scripts && /tmp/scripts/install_centos.sh && /tmp/scripts/install_deps.sh && rm -rf /tmp/scripts
#10 CACHED

#11 [4/5] RUN adduser --uid 1000 onnxruntimedev
#11 CACHED

#12 [5/5] WORKDIR /home/onnxruntimedev
#12 CACHED
```

###Reference
https://docs.docker.com/build/drivers/

---------

Co-authored-by: Yi Zhang <your@email.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fdwr fdwr fdwr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Honry @fdwr