Support Unsqueeze by Honry · Pull Request #12 · Honry/onnxruntime

Honry · 2023-05-18T13:27:13Z

No description provided.

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc

fdwr

Thanks Wanming. Minor comments.

fdwr · 2023-05-18T20:54:43Z

onnxruntime/core/providers/webnn/builders/helper.h

    {"Reshape", "reshape"},
    {"Resize", "resample2d"},
    {"Split", "split"},
-    {"Transpose", "transpose"}};


👍 Yeah, not putting the closing } on the same brace is better because then you can more clearly see additions.

fdwr · 2023-05-18T21:03:31Z

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc

+                                                 const Node& node,
+                                                 const logging::Logger& logger) const {
+  const auto& input_defs = node.InputDefs();
+  emscripten::val input = model_builder.GetOperand(input_defs[0]->Name());


Propose ORT_RETURN_IF_NOT(input_defs.size() < 1, "Unsqueeze has no input tensor"); since Definitions::input_defs as a std::vector<NodeArg*> has no bounds checking.

Or alternatively, check in the input count in the else branch of UnsqueezeOpBuilder::IsOpSupportedImpl:

if (node.SinceVersion() >= 13) { ... } else { if (input_defs.size() < 1) { ...

fdwr · 2023-05-18T21:06:03Z

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc

+
+  if (node.SinceVersion() >= 13) {
+    // Input axes is provided, use axes initializer data.
+    const auto& initializers(model_builder.GetInitializerTensors());


Suggested change

const auto& initializers(model_builder.GetInitializerTensors());

const auto& initializers = model_builder.GetInitializerTensors();

Minor suggestion. At first glance due to parentheses, this looks like it's calling a constructor to create an object, but then you notice the little & and realize it actually it's just assigning a reference.

fdwr · 2023-05-18T21:12:35Z

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc

+    const auto& initializers(model_builder.GetInitializerTensors());
+    const auto& axes_tensor = *initializers.at(input_defs[1]->Name());
+    Initializer axes_initializer(axes_tensor);
+    const auto axes_data_span = axes_initializer.DataAsSpan<int64_t>();


Does DataAsSpan validate the tensor type is actually int64_t first before casting? If not, we should validate before accessing the memory:

e.g.
ORT_RETURN_IF_NOT(axes_initializer.data_type() != ONNXTensorElementDataType::ONNX_TENSOR_ELEMENT_DATA_TYPE_INT64, "Unsqueeze axes should be int64 data type.");

Or maybe what you did in SliceOpBuilder::IsOpSupportedImpl:

if (shape_tensor.data_type() != ONNXTensorElementDataType::ONNX_TENSOR_ELEMENT_DATA_TYPE_INT64) { LOGS(logger, VERBOSE) << "The type of tensor's element data must be INT64."; return false; }

DataAsSpan does check the date type.

DataAsSpan does check the date type.

Cool.

Sorry for the silly question. For some reason I wasn't getting search results for DataAsSpan in VS code. 🤷‍♂️

ORT_ENFORCE(utils::IsPrimitiveDataType<T>(dtype_), "Tensor type mismatch. ", "T ", "!=", dtype_);

fdwr · 2023-05-18T21:15:45Z

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc

+// Operator support related
+
+bool UnsqueezeOpBuilder::IsOpSupportedImpl(const InitializedTensorSet& initializers, const Node& node,
+                                         const logging::Logger& logger) const {


Suggested change

const logging::Logger& logger) const {

const logging::Logger& logger) const {

fdwr · 2023-05-18T23:09:20Z

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc

+          [input_rank](int64_t axis) -> int32_t { return HandleNegativeAxis(axis, input_rank); });
+    }
+  }
+


Per our Teams message:

output_rank = input_rank + axes.size();
HandleNegativeAxis(axis, output_rank); });

Honry · 2023-05-19T02:48:53Z

@fdwr, comments addressed, PTAL again, thanks!

fdwr

Thanks Honry ⭐.

fdwr · 2023-05-19T03:10:01Z

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc

+    const auto& initializers(model_builder.GetInitializerTensors());
+    const auto& axes_tensor = *initializers.at(input_defs[1]->Name());
+    Initializer axes_initializer(axes_tensor);
+    const auto axes_data_span = axes_initializer.DataAsSpan<int64_t>();


Sorry for the silly question. For some reason I wasn't getting search results for DataAsSpan in VS code. 🤷‍♂️

ORT_ENFORCE(utils::IsPrimitiveDataType<T>(dtype_), "Tensor type mismatch. ", "T ", "!=", dtype_);

… 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 <[email protected]>

github-advanced-security bot found potential problems May 18, 2023

View reviewed changes

onnxruntime/core/providers/webnn/builders/impl/unsqueeze_op_builder.cc Fixed Show fixed Hide fixed

fdwr reviewed May 18, 2023

View reviewed changes

Honry added 2 commits May 19, 2023 10:46

Support Unsqueeze

6d35ac6

Addressed comments

e1bd8fe

Honry force-pushed the unsqueeze branch from 546ce7e to e1bd8fe Compare May 19, 2023 02:47

fdwr approved these changes May 19, 2023

View reviewed changes

Merge branch 'stable-diffusion' into unsqueeze

f84f383

Honry merged commit 2c7713b into stable-diffusion May 19, 2023

	const auto& initializers(model_builder.GetInitializerTensors());
	const auto& initializers = model_builder.GetInitializerTensors();

	const logging::Logger& logger) const {
	const logging::Logger& logger) const {

Comments

Conversation

Honry commented May 18, 2023

Uh oh!

Uh oh!

fdwr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdwr May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdwr May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Honry commented May 19, 2023

Uh oh!

fdwr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fdwr May 18, 2023 •

edited

Loading

fdwr May 19, 2023 •

edited

Loading