Plots duplicated when saving notebook repeatedly

[b11z]

I've noticed an issue, where each time a (classic) jupyter notebook is saved, any vega plot gets duplicated an additional time.

In troubleshooting, I can see that this line:

ipyvega/src/index.ts

Line 61 in 8c9bba9

if (imgIndex > -1 && jsIndex > -1 && imgIndex === jsIndex + 1) {

never triggers, causing a PNG to get appended to the outputs each time. It appears that the if condition doesn't trigger because imgIndex === jsIndex + 2. Looking at the saved notebook, this is caused by an empty text/plain output nestled between the javascript and PNG outputs:

   "outputs": [
    {
     "data": {
      "application/javascript": ...
     },
     "metadata": {
      "jupyter-vega": "#fb102a8f-bd0f-4c3b-a1a4-2176718b7100"
     },
    {
     "data": {
      "text/plain": []
     },
     "metadata": {},
     "output_type": "display_data"
    },
    {
     "data": {
      "image/png": ...
     },
     "metadata": {
      "jupyter-vega": "#fb102a8f-bd0f-4c3b-a1a4-2176718b7100"
     },
     "output_type": "display_data"
    }
   ],

In case it helps, here's a screenshot after saving (and reloading) the notebook multiple times:

I'm hoping this might be enough to point towards a potential solution. Thanks!

[domoritz]

Thanks for the bug report. Why do we even need all of these checks? Isn't it enough to just check whether the image already exists? Or actually, why do we check at all? When does it happen that we call render but don't want a new image? Someone needs to look into this a bit more.

[b11z]

I'm not at all an expert, but to attempt to answer these questions:

Isn't it enough to just check whether the image already exists?

Yeah, I'm wondering the same thing. Changing the conditional from:
if (imgIndex > -1 && jsIndex > -1 && imgIndex === jsIndex + 1)
to:
if (imgIndex > -1 && jsIndex > -1)
appears to fix the issue. But I'm not clear on what the motivation for the check is, and whether this change might break something else.

These set of conditionals were added all at once here: be19059

Or actually, why do we check at all? When does it happen that we call render but don't want a new image?

This function is called from the javascript output of the jupyter cell, which will have:

require(["nbextensions/jupyter-vega/index"], function(vega) {
  ...
  vega.render("#" + id, spec, type, opt, output_area);
}

I see it being called each time the page is reloaded, so I believe it must be idempotent if it has run before and modified the notebook document already.

[domoritz]

Thanks for looking into this.

I see it being called each time the page is reloaded, so I believe it must be idempotent if it has run before and modified the notebook document already.

So we need the check whether the image is already present. However, I think we only need to check for the image, and not the js. So if (imgIndex > -1) should be enough, imho.

Maybe @ellisonbg can help us why the original condition was written in the way it was.

[ellisonbg]

Yeah, this logic was super subtle and I am not surprised we have bugs in it. I can describe the different situations where this code gets called and what the desired behavior is:

• Already open notebook, cell runs for the first time, output empty. In this case the JS version should render and we should save the png version.
• Already open notebook, cell has been run, JS and PNG versions are present in the output bundle. In this case we should render the JS version and preserve the PNG version. This is really a re-rendering case that occurs when a notebook cell is copy/pasted/moved.
• Notebook opens with output bundle already having the JS/PNG. This is subtle because of the notebook trust question. If the notebook is trusted, we should render the JS, if not, the PNG.
• nbviewer, where both JS and PNG exist and we want to render the JS.

Question - does the additional image appear when the notebook is saved or when it is reopened after a save. I am somewhat wondering if there has been a change in how the notebook itself of behaving in these situations.

I don't remember why that imgIndex === jsIndex + 1 check is there, but I have a vague recollection that in some cases there could be another image in the output. What if that isn't the only output in a cell and one of the other outputs in an image? Or what if there are two Altair charts displayed in a single output?

[domoritz]

I actually can't reproduce the error in https://github.com/vega/ipyvega/blob/master/notebooks/VegaLite.ipynb. Can you help me create a small test case (ideally without Altair)?

[b11z]

Thanks for having a look! I can see the same behavior without Altair if I do:

from vega import VegaLite
from IPython.display import display

s = VegaLite({
  "$schema": "https://vega.github.io/schema/vega-lite/v3.json",
  "mark": "point",
  "encoding": {
    "y": {"type": "quantitative", "field": "x"},
    "x": {"type": "quantitative", "field": "y"}
  },
  "data": {
      "values": [
          {"x": 3, "y": 4},
          {"x": 0, "y": 0}
      ]
  }
})

display(s)
s

Click save on the notebook, then reload the page; you should see an additional copy of the plot each time you do so.

I had to take the above approach (displaying the plot and also returning it so it's displayed in the Out[] area) to reproduce, but you also see this behavior in altair even if not displaying twice this way.

[domoritz]

Thank you for the test case. I removed the imgIndex === jsIndex + 1 check and at least in my tests everything seems to work. I will make a release with this and see whether it's causing any issues.