During error conditions, we were currently putting an entire NodeDef information in the user facing error message. This makes it very hard to read and understand the error messages. We now just put the name in the message, and put the NodeDef information in the logs.

PiperOrigin-RevId: 217616833
2018-10-17 17:09:23 -07:00 · 2018-10-17 17:09:23 -07:00 · be409cac81
commit be409cac81
parent a54150fe95
17 changed files with 62 additions and 54 deletions
--- a/tensorflow/compiler/jit/deadness_analysis.cc
+++ b/tensorflow/compiler/jit/deadness_analysis.cc
@ -696,8 +696,8 @@ Status CreateMultipleNextIterationInputsError(Node* merge) {
    }
  }
  return errors::InvalidArgument(
-      "Multiple NextIteration inputs to merge node ", SummarizeNode(*merge),
-      ": \n", absl::StrJoin(backedges, "\n"),
+      "Multiple NextIteration inputs to merge node ",
+      FormatNodeForError(*merge), ": \n", absl::StrJoin(backedges, "\n"),
      "\nMerge nodes can have at most one incoming NextIteration edge.");
 }

--- a/tensorflow/compiler/tf2xla/graph_compiler.cc
+++ b/tensorflow/compiler/tf2xla/graph_compiler.cc
@ -164,7 +164,7 @@ Status GraphCompiler::Compile() {
      outputs[o] = op_context.release_output(o);
      if (outputs[o].tensor == nullptr) {
        return errors::Internal("Missing xla_context ", o, "-th output from ",
-                                SummarizeNode(*n));
+                                FormatNodeForError(*n));
      }
    }
  }
--- a/tensorflow/compiler/tf2xla/tf2xla.cc
+++ b/tensorflow/compiler/tf2xla/tf2xla.cc
@ -218,7 +218,7 @@ Status CollectArgNodes(const Graph& graph, std::vector<Node*>* arg_nodes) {
        const Node* dup = insert_result.first->second;
        return errors::InvalidArgument(
            "Multiple ", kArgOp, " nodes with index ", index, ", ",
-            n->DebugString(), " and ", dup->DebugString());
+            FormatNodeForError(*n), " and ", FormatNodeForError(*dup));
      }
    }
  }
--- a/tensorflow/compiler/tf2xla/xla_compilation_device.cc
+++ b/tensorflow/compiler/tf2xla/xla_compilation_device.cc
@ -92,7 +92,7 @@ Allocator* XlaCompilationDevice::GetAllocator(AllocatorAttributes attr) {
 void XlaCompilationDevice::Compute(OpKernel* op_kernel,
                                   OpKernelContext* context) {
  VLOG(4) << "XlaCompilationDevice::Compute "
-          << SummarizeNodeDef(op_kernel->def());
+          << FormatNodeDefForError(op_kernel->def());
  auto* b = XlaContext::Get(context).builder();
  xla::OpMetadata metadata;
  metadata.set_op_type(op_kernel->type_string());
--- a/tensorflow/compiler/tf2xla/xla_compiler_test.cc
+++ b/tensorflow/compiler/tf2xla/xla_compiler_test.cc
@ -354,8 +354,10 @@ TEST_F(XlaCompilerTest, HasSaneErrorOnNonCompileTimeConstantInputToReshape) {
  EXPECT_TRUE(
      absl::StrContains(status.error_message(), "depends on a parameter"))
      << status.error_message();
-  EXPECT_TRUE(
-      absl::StrContains(status.error_message(), "[[{{node C}} = Reshape"))
+  EXPECT_TRUE(absl::StrContains(status.error_message(), "{{node C}}"))
+      << status.error_message();
+  EXPECT_TRUE(absl::StrContains(status.error_message(),
+                                "must be a compile-time constant"))
      << status.error_message();
 }

--- a/tensorflow/core/common_runtime/eager/execute.cc
+++ b/tensorflow/core/common_runtime/eager/execute.cc
@ -197,10 +197,10 @@ Status SelectDevice(const NodeDef& ndef, EagerContext* ctx, Device** device) {
  TF_RETURN_IF_ERROR(SupportedDeviceTypesForNode(
      ctx->prioritized_device_type_list(), ndef, &final_devices));
  if (final_devices.empty()) {
-    return errors::Internal(
-        "Could not find valid device for node.\nNode: ", SummarizeNodeDef(ndef),
-        "\nAll kernels registered for op ", ndef.op(), " :\n",
-        KernelsRegisteredForOp(ndef.op()));
+    return errors::Internal("Could not find valid device for node.\nNode: ",
+                            FormatNodeDefForError(ndef),
+                            "\nAll kernels registered for op ", ndef.op(),
+                            " :\n", KernelsRegisteredForOp(ndef.op()));
  }
  for (Device* d : *ctx->devices()) {
    if (d->device_type() == final_devices[0].type_string()) {
@ -209,7 +209,7 @@ Status SelectDevice(const NodeDef& ndef, EagerContext* ctx, Device** device) {
    }
  }
  return errors::Unknown("Could not find a device for node ",
-                         SummarizeNodeDef(ndef));
+                         FormatNodeDefForError(ndef));
 }

 Status GetOutputDTypes(EagerOperation* op, DataTypeVector* output_dtypes) {
--- a/tensorflow/core/common_runtime/executor.cc
+++ b/tensorflow/core/common_runtime/executor.cc
@ -1976,7 +1976,7 @@ Status ExecutorState::ProcessOutputs(const NodeItem& item, OpKernelContext* ctx,
      // tensor value at i-th output.
      if (!IsSwitch(node) && !IsRecv(node)) {
        s.Update(errors::Internal("Missing ", i, "-th output from ",
-                                  SummarizeNode(*node)));
+                                  FormatNodeForError(*node)));
      }
    } else {
      Entry* out = &((*outputs)[i]);
@ -2030,7 +2030,7 @@ Status ExecutorState::ProcessOutputs(const NodeItem& item, OpKernelContext* ctx,
                                  DataTypeString(dtype),
                                  " does not match declared output type ",
                                  DataTypeString(item.output_type(i)),
-                                  " for node ", SummarizeNode(*node)));
+                                  " for node ", FormatNodeForError(*node)));
      }
    }
    if (!val.is_ref()) {
--- a/tensorflow/core/common_runtime/lower_if_while.cc
+++ b/tensorflow/core/common_runtime/lower_if_while.cc
@ -79,9 +79,9 @@ Status LowerIfWhilePass::Run(const GraphOptimizationPassOptions& options) {
        TF_RETURN_IF_ERROR(RewriteWhileNode(n, g, *flib));
      } else {
        return errors::Internal(
-            "Node:", n->name(), " of type ", n->type_string(), " has '",
-            LowerIfWhilePass::kLowerUsingSwitchMergeAttr,
-            "' attr set but it does not support lowering.\n", n->DebugString());
+            "Node ", FormatNodeForError(*n), " of type ", n->type_string(),
+            " has '", LowerIfWhilePass::kLowerUsingSwitchMergeAttr,
+            "' attr set but it does not support lowering.\n");
      }
    }
  }
--- a/tensorflow/core/common_runtime/memory_types.cc
+++ b/tensorflow/core/common_runtime/memory_types.cc
@ -96,11 +96,12 @@ Status ValidateMemoryTypes(const DeviceType& device_type, const Graph* g) {
        if (sm == dm) {
          return Status::OK();
        }
-        return errors::Internal(
-            "Memory type mismatch (", sm, " ", dm,
-            ") between :", e->src()->id(), ":", e->src_output(), " and ",
-            e->dst()->id(), ":", e->dst_input(), " : from ",
-            e->src()->DebugString(), " to ", e->dst()->DebugString());
+        return errors::Internal("Memory type mismatch (", sm, " ", dm,
+                                ") between :", e->src()->id(), ":",
+                                e->src_output(), " and ", e->dst()->id(), ":",
+                                e->dst_input(), " : from ",
+                                FormatNodeForError(*e->src()), " to ",
+                                FormatNodeForError(*e->dst()));
      });
 }

@ -209,7 +210,7 @@ Status MemoryTypeForOutput(const DeviceType& device_type, const Graph* g,
                                        &inp_mvec, &out_mvec));
  if (out_mvec.size() <= index) {
    return errors::Internal("Trying to get the memory type for ", index,
-                            "'th output of node ", n->DebugString(),
+                            "'th output of node ", FormatNodeForError(*n),
                            " that has only ", out_mvec.size(), " outputs");
  }
  *memory_type = out_mvec[index];
--- a/tensorflow/core/distributed_runtime/graph_mgr.cc
+++ b/tensorflow/core/distributed_runtime/graph_mgr.cc
@ -90,7 +90,7 @@ static Status ValidateGraphDefForDevices(const GraphDef& gdef) {
  for (const auto& ndef : gdef.node()) {
    if (!DeviceNameUtils::ParseFullName(ndef.device(), &parsed)) {
      return errors::InvalidArgument("Missing device name in: ",
-                                     SummarizeNodeDef(ndef));
+                                     FormatNodeDefForError(ndef));
    }
  }
  return Status::OK();
--- a/tensorflow/core/framework/function.cc
+++ b/tensorflow/core/framework/function.cc
@ -238,7 +238,8 @@ class FunctionInstantiationHelper {
        const auto* item = GetItemOrNull(input_name);
        if (item == nullptr) {
          return errors::InvalidArgument(
-              "input ", input_name, " is not found: ", SummarizeNodeDef(fnode));
+              "input ", input_name,
+              " is not found: ", FormatNodeDefForError(fnode));
        }
        if (item->dtypes.size() > dtypes.size() - j) {
          return errors::InvalidArgument("Input ", input_name, " too long for ",
@ -677,7 +678,8 @@ Status InstantiateFunction(const FunctionDef& fdef, AttrSlice attr_values,
    s = helper.BuildNodeOutputIndex(fdef.node_def(i), AttrSlice(&node_attrs[i]),
                                    result->nodes.size() + i);
    if (!s.ok()) {
-      errors::AppendToMessage(&s, "In ", SummarizeNodeDef(fdef.node_def(i)));
+      errors::AppendToMessage(&s, "In ",
+                              FormatNodeDefForError(fdef.node_def(i)));
      return s;
    }
  }
@ -685,7 +687,8 @@ Status InstantiateFunction(const FunctionDef& fdef, AttrSlice attr_values,
  for (int i = 0; i < fdef.node_def_size(); ++i) {
    s = helper.InstantiateNode(fdef.node_def(i), AttrSlice(&node_attrs[i]));
    if (!s.ok()) {
-      errors::AppendToMessage(&s, "In ", SummarizeNodeDef(fdef.node_def(i)));
+      errors::AppendToMessage(&s, "In ",
+                              FormatNodeDefForError(fdef.node_def(i)));
      return s;
    }
  }
--- a/tensorflow/core/framework/graph_def_util.cc
+++ b/tensorflow/core/framework/graph_def_util.cc
@ -103,7 +103,7 @@ static Status RemoveNewDefaultAttrsFromNodeDef(
        return errors::InvalidArgument(
            "Attr '", attr.first,
            "' missing in producer's OpDef: ", SummarizeOpDef(*producer_op_def),
-            " but found in node: ", SummarizeNodeDef(*node_def));
+            " but found in node: ", FormatNodeDefForError(*node_def));
      }
      // ...and it has the same value as the default in producer,
      if (producer_attr_def->has_default_value() &&
--- a/tensorflow/core/framework/node_def_util.cc
+++ b/tensorflow/core/framework/node_def_util.cc
@ -86,8 +86,8 @@ string AttrSlice::SummarizeNode() const {
 string SummarizeNode(const Node& node) { return SummarizeNodeDef(node.def()); }

 string SummarizeNodeDef(const NodeDef& node_def) {
-  string ret = strings::StrCat(FormatNodeDefForError(node_def), " = ",
-                               node_def.op(), "[");
+  string ret = strings::StrCat(errors::FormatNodeNameForError(node_def.name()),
+                               " = ", node_def.op(), "[");
  strings::StrAppend(&ret, SummarizeAttrsHelper(node_def, node_def.device()));
  strings::StrAppend(&ret, "](");

@ -107,6 +107,7 @@ string FormatNodeForError(const Node& node) {
 }

 string FormatNodeDefForError(const NodeDef& node_def) {
+  LOG(ERROR) << "Error in the node: " << SummarizeNodeDef(node_def);
  return errors::FormatNodeNameForError(node_def.name());
 }

@ -419,9 +420,9 @@ Status NumOutputsForNode(const NodeDef& node_def, const OpDef& op_def,

 Status ValidateNodeDef(const NodeDef& node_def, const OpDef& op_def) {
  if (node_def.op() != op_def.name()) {
-    return errors::InvalidArgument("NodeDef op '", node_def.op(),
-                                   "' does not match ", SummarizeOpDef(op_def),
-                                   "; NodeDef: ", SummarizeNodeDef(node_def));
+    return errors::InvalidArgument(
+        "NodeDef op '", node_def.op(), "' does not match ",
+        SummarizeOpDef(op_def), "; NodeDef: ", FormatNodeDefForError(node_def));
  }

  bool seen_control = false;
@ -431,14 +432,14 @@ Status ValidateNodeDef(const NodeDef& node_def, const OpDef& op_def) {
    if (str_util::StartsWith(input, "^")) {
      seen_control = true;
      if (input.find(':') != string::npos) {
-        return errors::InvalidArgument(
-            "Control input '", input,
-            "' must not have ':' in NodeDef: ", SummarizeNodeDef(node_def));
+        return errors::InvalidArgument("Control input '", input,
+                                       "' must not have ':' in NodeDef: ",
+                                       FormatNodeDefForError(node_def));
      }
    } else if (seen_control) {
-      return errors::InvalidArgument(
-          "Non-control input '", input,
-          "' after control input in NodeDef: ", SummarizeNodeDef(node_def));
+      return errors::InvalidArgument("Non-control input '", input,
+                                     "' after control input in NodeDef: ",
+                                     FormatNodeDefForError(node_def));
    } else {
      ++num_inputs;
    }
@ -468,13 +469,14 @@ Status ValidateNodeDef(const NodeDef& node_def, const OpDef& op_def) {
      // the binary producing it.
      return errors::InvalidArgument(
          "NodeDef mentions attr '", attr.first, "' not in ",
-          SummarizeOpDef(op_def), "; NodeDef: ", SummarizeNodeDef(node_def),
+          SummarizeOpDef(op_def),
+          "; NodeDef: ", FormatNodeDefForError(node_def),
          ". (Check whether your GraphDef-interpreting binary is up to date "
          "with your GraphDef-generating binary.).");
    }
    TF_RETURN_WITH_CONTEXT_IF_ERROR(
        ValidateAttrValue(attr.second, *iter->second),
-        "; NodeDef: ", SummarizeNodeDef(node_def), "; ",
+        "; NodeDef: ", FormatNodeDefForError(node_def), "; ",
        SummarizeOpDef(op_def));
    // Keep track of which attr names have (not) been found in the NodeDef.
    op_attrs.erase(iter);
@ -487,10 +489,10 @@ Status ValidateNodeDef(const NodeDef& node_def, const OpDef& op_def) {
      if (!attrs.empty()) strings::StrAppend(&attrs, "', '");
      strings::StrAppend(&attrs, attr_pair.first);
    }
-    return errors::InvalidArgument("NodeDef missing attr",
-                                   op_attrs.size() == 1 ? " '" : "s '", attrs,
-                                   "' from ", SummarizeOpDef(op_def),
-                                   "; NodeDef: ", SummarizeNodeDef(node_def));
+    return errors::InvalidArgument(
+        "NodeDef missing attr", op_attrs.size() == 1 ? " '" : "s '", attrs,
+        "' from ", SummarizeOpDef(op_def),
+        "; NodeDef: ", FormatNodeDefForError(node_def));
  }

  // Validate the number of inputs.
@ -501,7 +503,7 @@ Status ValidateNodeDef(const NodeDef& node_def, const OpDef& op_def) {
    return errors::InvalidArgument(
        "NodeDef expected inputs '", DataTypeVectorString(inputs),
        "' do not match ", num_inputs, " inputs specified; ",
-        SummarizeOpDef(op_def), "; NodeDef: ", SummarizeNodeDef(node_def));
+        SummarizeOpDef(op_def), "; NodeDef: ", FormatNodeDefForError(node_def));
  }

  return Status::OK();
@ -657,7 +659,7 @@ Status ValidateExternalNodeDefSyntax(const NodeDef& node_def) {
 Status AttachDef(const Status& status, const NodeDef& node_def) {
  Status ret = status;
  errors::AppendToMessage(
-      &ret, strings::StrCat(" [[", SummarizeNodeDef(node_def), "]]"));
+      &ret, strings::StrCat(" [[", FormatNodeDefForError(node_def), "]]"));
  return ret;
 }

--- a/tensorflow/core/framework/op_kernel.cc
+++ b/tensorflow/core/framework/op_kernel.cc
@ -1033,7 +1033,7 @@ Status FindKernelRegistration(const DeviceType& device_type,
      if (*reg != nullptr) {
        return errors::InvalidArgument(
            "Multiple OpKernel registrations match NodeDef '",
-            SummarizeNodeDef(node_def), "': '",
+            FormatNodeDefForError(node_def), "': '",
            ProtoShortDebugString((*reg)->def), "' and '",
            ProtoShortDebugString(iter->second.def), "'");
      }
@ -1058,7 +1058,7 @@ Status FindKernelDef(const DeviceType& device_type, const NodeDef& node_def,
    Status s = errors::NotFound(
        "No registered '", node_def.op(), "' OpKernel for ",
        DeviceTypeString(device_type), " devices compatible with node ",
-        SummarizeNodeDef(node_def));
+        FormatNodeDefForError(node_def));
    if (was_attr_mismatch) {
      errors::AppendToMessage(
          &s, " (OpKernel was found, but attributes didn't match)");
@ -1184,7 +1184,7 @@ Status CreateOpKernel(DeviceType device_type, DeviceBase* device,
    s.Update(errors::NotFound("No registered '", node_def.op(),
                              "' OpKernel for ", DeviceTypeString(device_type),
                              " devices compatible with node ",
-                              SummarizeNodeDef(node_def)));
+                              FormatNodeDefForError(node_def)));
    if (was_attr_mismatch) {
      errors::AppendToMessage(
          &s, " (OpKernel was found, but attributes didn't match)");
@ -1199,7 +1199,7 @@ Status CreateOpKernel(DeviceType device_type, DeviceBase* device,
  DataTypeVector outputs;
  s.Update(InOutTypesForNode(node_def, *op_def, &inputs, &outputs));
  if (!s.ok()) {
-    errors::AppendToMessage(&s, " for node: ", SummarizeNodeDef(node_def));
+    errors::AppendToMessage(&s, " for node: ", FormatNodeDefForError(node_def));
    return s;
  }

--- a/tensorflow/core/graph/graph.cc
+++ b/tensorflow/core/graph/graph.cc
@ -529,7 +529,7 @@ Status Graph::UpdateEdge(Node* new_src, int new_src_index, Node* dst,
  const Edge* e = FindEdge(dst, dst_index);
  if (e == nullptr) {
    return errors::InvalidArgument("Couldn't find edge to ",
-                                   dst->DebugString());
+                                   FormatNodeForError(*dst));
  }
  RemoveEdge(e);
  AddEdge(new_src, new_src_index, dst, dst_index);
--- a/tensorflow/core/graph/node_builder.cc
+++ b/tensorflow/core/graph/node_builder.cc
@ -143,7 +143,7 @@ void NodeBuilder::AddIndexError(const Node* node, int i) {
    errors_.emplace_back(strings::StrCat(
        "Attempt to add output ", i, " of ", node->name(), " not in range [0, ",
        node->num_outputs(), ") to node with type ",
-        def_builder_.op_def().name(), ". Node: ", node->DebugString()));
+        def_builder_.op_def().name(), ". Node: ", FormatNodeForError(*node)));
  }
 }

--- a/tensorflow/python/client/session.py
+++ b/tensorflow/python/client/session.py
@ -1283,7 +1283,7 @@ class BaseSession(SessionInterface):
  # Old format: [[Node: <node_name> = ...]]
  # New format: [[{{node <node_name>}} = ...]]
  _NODEDEF_NAME_RE = re.compile(
-      r'\[\[(Node: )?(\{\{node )?([^\} ]*)(\}\})?\s*=')
+      r'\[\[(Node: )?(\{\{node )?([^\} ]*)(\}\})?\s*=*')

  def _do_run(self, handle, target_list, fetch_list, feed_dict, options,
              run_metadata):