Graph info panel is broken #3557

stephanwlee · 2020-04-28T07:01:09Z

Symptom: after the #3476 refactor, the graph info panel is broken.

Cause:
After the change, the iron-list dom-module is missing from the document. When inspecting the elements more carefully, it appears like following:

    <div id="numeric-alerts-table-container">
          <table id="numeric-alerts-table">
            <thead>
              <tr>
                <th>First Offense</th>
                <th>Tensor (Device)</th>
                <th id="event-counts-th">Event Counts</th>
              </tr>
            </thead>
            <tbody id="numeric-alerts-body"></tbody>
          </table>
        </div>
      </div>
      <template is="dom-if" if="[[!_hasDebuggerNumericAlerts(debuggerNumericAlerts)]]">
        <p class="no-numeric-alerts-notification">
          No numeric alerts so far. That is likely good. Alerts indicate the
          presence of NaN or (+/-) Infinity values, which may be concerning.

<dom-module id="iron-list">
  <template>
    <style>

As you can notice, the DOM is quite broken. There are multiple closing tags from tf-graph-debugger-data-card are missing.

Upon more careful debugging, I have found out that the Vulcanization is actually responsible for such structure.

For instance, <p>foo</p> gets compiled into <p>foo. Similarly,

<template>
  <p>foo</p>
</template>
<dom-module id="iron-list">
</dom-module

<!-- gets compiled to -->
<template>
  <p>foo
</template>

<dom-module id="iron-list">
</dom-module

When debugged further, I found that there are some heuristic on HTML5Printer of jsoup that we use which omits closing tag of certain tags when they are the last one in current level https://github.com/bazelbuild/rules_closure/blob/d1c0bb4862882e0444ee478aa2091d41a4df084e/java/org/jsoup/nodes/Html5Printer.java#L209-L214.

I think the intention was good but if you parse then print below code using jsoup, you get quite wrong code.

<div>
  <p>foo
</div>

<!-- converts to -->
<div>
  <p>foo

There are three ways to move forward:

fix jsoup not to wrongfully remove closing tag of parent node
never do multiple passes of jsoup parse and print
use genrule to inline polymer_lib into tensorboard.html since inlining is all we need.

cc @bmd3k

The text was updated successfully, but these errors were encountered:

stephanwlee · 2020-05-01T06:21:16Z

I think I understand the issue better now, @bmd3k.

When we are iterating and flattening HTML, we put the document in place of the link elements: https://github.com/tensorflow/tensorboard/blob/master/tensorboard/java/org/tensorflow/tensorboard/vulcanize/Vulcanize.java#L324

This results in a document like:

<html>
  <head>
    <document>
       <head>
        ...
    </document>
  </head>
</html>

Of course, <document> is non-sensical. Our printer, basically knows to ignore these elements (https://github.com/bazelbuild/rules_closure/blob/d1c0bb4862882e0444ee478aa2091d41a4df084e/java/org/jsoup/nodes/Html5Printer.java#L121-L123) but the default serialization does not. As a result, you see HTML stringified with awkward <#root> elements throughout.

I tried to see if we can not replace with the document but replace the link element with <head> and <body>. Sadly, our webpath logic strongly depends on the document being injected:

tensorboard/tensorboard/java/org/tensorflow/tensorboard/vulcanize/Vulcanize.java

Lines 308 to 309 in e6c4355

    
           if (node instanceof Document) { 
        
             stack.remove(stack.size() - 1);

Ideas that I have right now is a post-process step on the resulting document to remove all the intermediate documents before writing it out since we cannot pass our own traverser onto the printer (https://github.com/jhy/jsoup/blob/89580cc3d25d0d89ac1f46b349e5cd315883dc79/src/main/java/org/jsoup/nodes/Node.java#L606)

The issue is better described in tensorflow#3557. Please refer to it. Our HTML module traversal relies on the `document` so we cannot create the document without nested document in one go. Instead, we do a post processing of the document to prune away all the `document` so the default jsoup's printer can print them without awkward `<#root>` elements. index.html size difference: ``` 555323 before 554749 after ```

The issue is better described in #3557. Please refer to it. Our HTML module traversal relies on the document so we cannot create the document without nested document in one go. Instead, we do a post processing of the document to prune away all the document so the default jsoup's printer can print them without awkward <#root> elements.

The issue is better described in tensorflow#3557. Please refer to it. Our HTML module traversal relies on the document so we cannot create the document without nested document in one go. Instead, we do a post processing of the document to prune away all the document so the default jsoup's printer can print them without awkward <#root> elements.

The issue is better described in #3557. Please refer to it. Our HTML module traversal relies on the document so we cannot create the document without nested document in one go. Instead, we do a post processing of the document to prune away all the document so the default jsoup's printer can print them without awkward <#root> elements.

@stephanwlee

… link importing (#3723) * Motivation for features / changes * Roll forward #3582 with change, in order to fix #3557. * The solution is primarily due to @stephanwlee. When such midway imports are found, a `RuntimeException` is thrown with an error message describing the cause and the offending element.

stephanwlee added plugin:graph type:build/install labels Apr 28, 2020

amahendrakar assigned amahendrakar and gowthamkpr and unassigned amahendrakar Apr 29, 2020

gowthamkpr removed their assignment Apr 30, 2020

stephanwlee mentioned this issue May 5, 2020

vulc: fix mangled DOM due to a bug in the printer #3582

Merged

stephanwlee closed this as completed in #3582 May 7, 2020

stephanwlee mentioned this issue May 21, 2020

Revert "build: refactor ng_polymer_lib to be reused." #3664

Closed

This was referenced Jun 9, 2020

Roll forward vulcanization change (#3582) with check for link import outside head #3722

Closed

Roll forward vulcanization printer change with check for outside-head link importing #3723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graph info panel is broken #3557

Graph info panel is broken #3557

stephanwlee commented Apr 28, 2020 •

edited

Loading

stephanwlee commented May 1, 2020

Graph info panel is broken #3557

Graph info panel is broken #3557

Comments

stephanwlee commented Apr 28, 2020 • edited Loading

stephanwlee commented May 1, 2020

stephanwlee commented Apr 28, 2020 •

edited

Loading