expose graph node name returning non-zero status code by hariharans29 · Pull Request #714 · microsoft/onnxruntime

hariharans29 · 2019-03-27T03:05:18Z

Helps debugging if the graph node name that returns non-zero status code is available.

In case of exceptions from within node kernels (by using ORT_THROW), the stack trace should contain atleast the kernel name it failed at (if not the graph node name), so avoiding the (potentially) costly try...catch for now

skottmckay · 2019-03-27T03:13:39Z

onnxruntime/core/framework/sequential_executor.cc

+    if (!compute_status.IsOK()) {
+      LOGS(logger, ERROR) << "Non-zero status code returned while running Node: ",
+                             p_op_kernel->Node().Name(), " Status Message: ", compute_status.ErrorMessage();
+      return ORT_MAKE_STATUS(ONNXRUNTIME, FAIL, "Non-zero status code returned while running Node: ",


Not sure if anything external depends on the failure category or code. You may want to maintain those instead of always overwriting with ONNXRUNTIME and FAIL. #Closed

That makes sense, thanks, fixed. #Closed

snnn · 2019-03-27T09:05:06Z

what if the name is empty?

hariharans29 · 2019-03-27T19:55:14Z

what if the name is empty?

@snnn - I actually assumed handling was already done while resolving the graph (my bad), I have a light-weight mechanism to generate a node name if empty (based on op type). If op type is empty, it should break elsewhere as kernel resolving cannot happen, although it might be worthwhile to check missing op type at graph resolution time as well perhaps

pranavsharma · 2019-03-27T20:14:50Z

what if the name is empty?

@snnn - I actually assumed handling was already done while resolving the graph (my bad), I have a light-weight mechanism to generate a node name if empty (based on op type). If op type is empty, it should break elsewhere as kernel resolving cannot happen, although it might be worthwhile to check missing op type at graph resolution time as well perhaps

The scope of this PR is to print the node name when available.

pranavsharma · 2019-03-27T20:16:56Z

onnxruntime/core/graph/graph.cc

+
+  std::string node_name = node_proto.name();
+  if (node_name.empty())
+    node_name = GenerateNodeName("unnamed_" + op_type + "_" +


Why do we need this? This name won't be visible on Netron any way when debugging.

maybe not, but atleast it helps if we can visualize the serialized proto (not sure if this is possible) and the information here would atleast tell us that a specific op type (offset by the specific count) malfunctioned (as opposed to having no information at all)

pranavsharma

LGTM cc @linkerzhang

pranavsharma · 2019-03-27T23:10:46Z

onnxruntime/core/graph/graph.cc

+  else
+    current_op_type_count = ++iter->second;
+
+  std::string node_name = node_proto.name();


std::string&

pranavsharma · 2019-03-27T23:10:56Z

onnxruntime/core/graph/graph.cc


-  return AddNode(node_proto.name(),
+  size_t current_op_type_count = 1;
+  const auto op_type = node_proto.op_type();


const auto&

skottmckay · 2019-03-31T21:26:17Z

include/onnxruntime/core/graph/graph.h

  Node& AddNode(const ONNX_NAMESPACE::NodeProto& node_proto,
-                const ArgNameToTypeMap& name_to_type);
+                const ArgNameToTypeMap& name_to_type,
+                TypeToCountMap& type_to_count_map);


TypeToCountMap& type_to_count_map [](start = 16, length = 33)

Not a huge fan of the API taking this in order to create a hopefully unique name for the node as it's a bit obtuse. Possibly not expected behaviour for the name to be changed during AddNode either.

Why do we need this new approach?

I understand your concern regarding this. But don't you think it adds a little bit of value in terms of node debuggability in case of missing node names ? -

It gives information regarding the malfunctioning node (in terms of op_type + chronological order in the serialized proto).

It is almost always guaranteed to be unique as the name is a function of op_type + order it is seen.

As far as API change goes, correct me if I am wrong - I think the graph building methods are not currently being exposed to end-users anyway

Reverted the this aspect of the change

pranavsharma · 2019-04-05T03:44:17Z

onnxruntime/core/framework/sequential_executor.cc

+
+    const auto& compute_status = p_op_kernel->Compute(&op_kernel_context);
+    if (!compute_status.IsOK()) {
+      std::string msg_string =


it's better to use ostringstream when you're constructing a string from multiple strings. the + operators will make a copy. even if it doesn't matter so much in this case since we're doing it in the error scenario, it's a good habit.

…icrosoft#714) * Added support for 2025.2 and SimplifiedLayerNormalization op * [OVEP] Update OV version to 2025.2.0 * Revert "[OVEP] Update OV version to 2025.2.0" This reverts commit d129250.

hariharans29 added 2 commits March 26, 2019 19:55

Initial commit

f1c1fd7

Formatting

f06d22c

hariharans29 requested a review from a team as a code owner March 27, 2019 03:05

skottmckay reviewed Mar 27, 2019

View reviewed changes

hariharans29 added 4 commits March 26, 2019 23:35

PR feedback

56b85ba

Fix build break

ec80238

Revert formatting change in cs file

7dbd15b

Revert some cs test changes

af48da9

hariharans29 requested review from jignparm and shahasad March 27, 2019 07:50

skottmckay previously approved these changes Mar 27, 2019

View reviewed changes

snnn previously approved these changes Mar 27, 2019

View reviewed changes

PR feedback

cfeabfe

hariharans29 dismissed stale reviews from snnn and skottmckay via cfeabfe March 27, 2019 19:49

Formatting

831adc8

Formatting

1b74d51

pranavsharma reviewed Mar 27, 2019

View reviewed changes

More changes

cbda159

pranavsharma reviewed Mar 27, 2019

View reviewed changes

hariharans29 added 4 commits March 27, 2019 17:08

PR feedback

4204f01

Fix build break

6c4f987

Fix build break

2be28f0

More changes

0a26c69

hariharans29 closed this Mar 28, 2019

hariharans29 reopened this Mar 28, 2019

Nits

77a2869

hariharans29 added 2 commits March 27, 2019 19:05

Formatting

9019764

Nits

5060879

skottmckay reviewed Mar 31, 2019

View reviewed changes

hariharans29 added 2 commits April 4, 2019 16:27

Revert some files to as they are in master

d063351

Revert files

7437bf8

hariharans29 closed this Apr 4, 2019

hariharans29 reopened this Apr 4, 2019

hariharans29 added 2 commits April 4, 2019 16:35

Merge branch 'master' into errorMessageNodeName

1e4f7c9

Revert files

5c12086

pranavsharma reviewed Apr 5, 2019

View reviewed changes

hariharans29 added 2 commits April 4, 2019 21:07

PR feedback

c782134

Nit fix

52fc8bf

pranavsharma approved these changes Apr 5, 2019

View reviewed changes

hariharans29 merged commit ffd9071 into master Apr 5, 2019

hariharans29 deleted the errorMessageNodeName branch April 5, 2019 19:51

Comments

Conversation

hariharans29 commented Mar 27, 2019

Uh oh!

skottmckay Mar 27, 2019 • 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!

hariharans29 Mar 27, 2019 • edited by skottmckay Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snnn commented Mar 27, 2019

Uh oh!

hariharans29 commented Mar 27, 2019

Uh oh!

pranavsharma commented Mar 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavsharma left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skottmckay Mar 27, 2019 •

edited

Loading

hariharans29 Mar 27, 2019 •

edited by skottmckay

Loading