- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.7k
 
Move What-If Tool into an iframe in Jupyter widget #2216
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.
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
Conversation
| 
           @tolga-b please review  | 
    
| 
           
  | 
    
          
 This only effects the Jupyter version. No changes to colab version of WitWidget, which already works in the same notebook as TFMA visualizations, as colab uses iframes for each output cell already.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving because the risk mentioned inline is not new in this PR, but
I do want to make sure that you’re aware of it.
| const templateLocation = | ||
| window.__webpack_public_path__ + 'wit_jupyter.html'; | ||
| window.__webpack_public_path__ + 'wit_jupyter.html'; | ||
| const src = `<link rel="import" href="${templateLocation}"> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect that this will break very soon. HTML imports are deprecated and
will be removed in Chrome M75 (we’re on M74 right now). TensorBoard’s
HTML imports are compiled statically, so we don’t immediately have this
problem, but it looks like this is using HTML imports at runtime. Have
you tested this on Chrome canary?
Depending on what exactly is in wit_jupyter.html, you may be able to
achieve the same behavior by, within the iframe, (1) fetching that
page, (2) setting the document.body.innerHTML to its source, and (3)
rehydrating <script> elements. You can see how we do that for the
%tensorboard Colab widget here:
tensorboard/tensorboard/notebook.py
Lines 314 to 364 in 1925d06
| shell = """ | |
| <div id="root"></div> | |
| <script> | |
| (function() { | |
| window.TENSORBOARD_ENV = window.TENSORBOARD_ENV || {}; | |
| window.TENSORBOARD_ENV["IN_COLAB"] = true; | |
| document.querySelector("base").href = "https://localhost:%PORT%"; | |
| function fixUpTensorboard(root) { | |
| const tftb = root.querySelector("tf-tensorboard"); | |
| // Disable the fragment manipulation behavior in Colab. Not | |
| // only is the behavior not useful (as the iframe's location | |
| // is not visible to the user), it causes TensorBoard's usage | |
| // of `window.replace` to navigate away from the page and to | |
| // the `localhost:<port>` URL specified by the base URI, which | |
| // in turn causes the frame to (likely) crash. | |
| tftb.removeAttribute("use-hash"); | |
| } | |
| function executeAllScripts(root) { | |
| // When `script` elements are inserted into the DOM by | |
| // assigning to an element's `innerHTML`, the scripts are not | |
| // executed. Thus, we manually re-insert these scripts so that | |
| // TensorBoard can initialize itself. | |
| for (const script of root.querySelectorAll("script")) { | |
| const newScript = document.createElement("script"); | |
| newScript.type = script.type; | |
| newScript.textContent = script.textContent; | |
| root.appendChild(newScript); | |
| script.remove(); | |
| } | |
| } | |
| function setHeight(root, height) { | |
| // We set the height dynamically after the TensorBoard UI has | |
| // been initialized. This avoids an intermediate state in | |
| // which the container plus the UI become taller than the | |
| // final width and cause the Colab output frame to be | |
| // permanently resized, eventually leading to an empty | |
| // vertical gap below the TensorBoard UI. It's not clear | |
| // exactly what causes this problematic intermediate state, | |
| // but setting the height late seems to fix it. | |
| root.style.height = `${height}px`; | |
| } | |
| const root = document.getElementById("root"); | |
| fetch(".") | |
| .then((x) => x.text()) | |
| .then((html) => void (root.innerHTML = html)) | |
| .then(() => fixUpTensorboard(root)) | |
| .then(() => executeAllScripts(root)) | |
| .then(() => setHeight(root, %HEIGHT%)); | |
| })(); | |
| </script> | |
| """.replace("%PORT%", "%d" % port).replace("%HEIGHT%", "%d" % height) | 
In order for WitWidget to live in a Jupyter notebook without interfering with other widgets that also use Polymer, we need to load it inside an iframe. Otherwise for examples the TFMA visualizations and WitWidget cannot live in the same notebook without failing, due to their vulcanized HTML files for their widgets both containing polymer dependencies (since the browser will fail on the loading of those deps a second time).
Moved WIT HTML element and its vulcanized dependency to inside an iframe.
Updated wit.js code to ensure WIT is fully loaded inside iframe before trying to use it.
Update WIT element to use correct font, as without this, inside iframe it reverts to default browser fonts.
Update WIT element to auto-infer when a model is set, if examples have already been loaded. Previously this only happened if a model was set and THEN examples were set. But it should auto-infer when these are set in either order.
No visual changes, but included screenshot of WIT loading inside iframe in Jupyter
pip build/install with this PR. Run WIT inside Jupyter notebook with model/data, ensure all functionality and visual correctness.