search

jupyter / jupyter-sphinx

Sphinx extension for rendering of Jupyter interactive widgets.
https://jupyter-sphinx.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
187 stars 65 forks source link

Fix for compat for ipywidgets and ipynb #196

Open bnouvelbmll opened 2 years ago

bnouvelbmll commented 2 years ago

We had issues with some serialised state from ipywidgets in notebook.

It was working fine when we using nbsphinx and old pinned dependencies - however when updating to latest sphinx and packages an exception was raised here. It seems it was excepted but that the wrong exception was caught - and that the exact type of exception to capture depends on other dependencies. I believe this change should be reasonable.

bnouvelbmll commented 2 years ago

Obviously, I don't know what motivate the comment just below, and we would need to remove this comment if we go with the PR.

akhmerov commented 2 years ago

This is unexpected: as far as I understand, if there are widgets in the notebook, the notebook must contain widget state. Do you have a minimal reproducible example? Can you write the notebook to disk around that point and share it?

bnouvelbmll commented 2 years ago

Sure, let me find the notebook that causes the issue, so we can clarify the root cause of the issue.

On Thu, 10 Feb 2022 at 14:56, Anton Akhmerov @.***> wrote:

This is unexpected: as far as I understand, if there are widgets in the notebook, the notebook must contain widget state. Do you have a minimal reproducible example? Can you write the notebook to disk around that point and share it?

— Reply to this email directly, view it on GitHub https://github.com/jupyter/jupyter-sphinx/pull/196#issuecomment-1035017881, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJPMSPHKB5M2RCIXCDPAD6LU2PG3TANCNFSM5OA22YKQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

bnouvelbmll commented 2 years ago

Actually, the notebooks had widgets which had failed to render due to the fact the notebook had apparently been rerendered by someone who did not have the extension installed. We had quite a lot of notebooks in our sphinx and we had not noticed this issue.

Whilst the exception has pointed to an issue in the notebook, a crash with KeyError when trying to render a large doc repo with sphinx is not really expected and the message not really helpful to most users to understand the cause of the issue.

Sphinx generally does not crash for tech errors, broken references, etc.. it simply issues warnings. I am thinking that it would be nicer here to be able raise a warning - that can be controlled by warnings control or to directly log a message. I will see if I can amend my PR to do this - as long as you are fine with this change.

On Thu, 10 Feb 2022 at 16:06, Bertrand Nouvel @.***> wrote:

Sure, let me find the notebook that causes the issue, so we can clarify the root cause of the issue.

On Thu, 10 Feb 2022 at 14:56, Anton Akhmerov @.***> wrote:

This is unexpected: as far as I understand, if there are widgets in the notebook, the notebook must contain widget state. Do you have a minimal reproducible example? Can you write the notebook to disk around that point and share it?

— Reply to this email directly, view it on GitHub https://github.com/jupyter/jupyter-sphinx/pull/196#issuecomment-1035017881, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJPMSPHKB5M2RCIXCDPAD6LU2PG3TANCNFSM5OA22YKQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

akhmerov commented 2 years ago

I agree that sphinx generally tends to not abort on data inconsistencies. Here, however, it is not clear to me how the error may occur and whether it isn't an actual bug. I believe that jupyter widgets must always write widget state whenever widgets are present. If that is correct, then this not happening would be an actual bug in the widget library and the way it communicates with the kernel, and shouldn't be treated like a user input problem. Basically something should create a widgets metadata entry in the notebook, but then not write widget state into this metadata.

I don't think the error message in the PR corresponds to the situation I described, and therefore wouldn't help debug what is happening. Can you share more information about the conditions under which the problem occurs?

bnouvelbmll commented 2 years ago

Hi Anton,

You are right that normally the widget state should have created:

The typical cells that were causing issues in the notebook were structured like this:

As indicated it suggests that the python part of ipywidgets was installed but not the javascript part when the notebook was initially rendered. This is certainly an issue with the notebook - the issue for jupyter-sphinx is just to make it clear to the user that something is wrong inside of the notebook.

You obviously know the code base much more than I do - you may find a better way to help users to identify the nature of the issue than the one I have suggested. For instance, I was not sure if there was a way to indicate the name of the notebook from where the exception was raised - being able to point at the notebook and to put an exact error message would already help a lot.

Kind regards,

Bertrand

On Sat, 12 Feb 2022 at 14:51, Anton Akhmerov @.***> wrote:

I agree that sphinx generally tends to not abort on data inconsistencies. Here, however, it is not clear to me how the error may occur and whether it isn't an actual bug. I believe that jupyter widgets must always write widget state whenever widgets are present. If that is correct, then this not happening would be an actual bug in the widget library and the way it communicates with the kernel, and shouldn't be treated like a user input problem. Basically something should create a widgets metadata entry in the notebook, but then not write widget state into this metadata.

I don't think the error message in the PR corresponds to the situation I described, and therefore wouldn't help debug what is happening. Can you share more information about the conditions under which the problem occurs?

— Reply to this email directly, view it on GitHub https://github.com/jupyter/jupyter-sphinx/pull/196#issuecomment-1037246860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJPMSPBOAFM5567Y4V6EGO3U2ZXYLANCNFSM5OA22YKQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

akhmerov commented 2 years ago

This is weird: jupyter-sphinx does execution using nbclient, which doesn't have a javascript side and never talks with javascript. How are you using a notebook file wit jupyter-sphinx in the first place?

bnouvelbmll commented 2 years ago

My bad that was not the issue - the issue is in the metadata of the notebook not on the cell that are rendered:

So, using jq. I can see two type of headers on with the mimetype before the state in widgets ( which I believe are aligned with the code:

{
  "kernelspec": {
    "display_name": "Python 3",
    "language": "python",
    "name": "python3"
  },
  "language_info": {
    "codemirror_mode": {
      "name": "ipython",
      "version": 3
    },
    "file_extension": ".py",
    "mimetype": "text/x-python",
    "name": "python",
    "nbconvert_exporter": "python",
    "pygments_lexer": "ipython3",
    "version": "3.6.6"
  },
  "widgets": {
    "application/vnd.jupyter.widget-state+json": {
      "state": {
        "0003c36486af462eaad389ed91fe3314": {
          "model_module": ***@***.***/base",
          "model_module_version": "1.0.0",
          "model_name": "LayoutModel",
          "state": {}
        },
        "00376cf2c7114633ba2c9fbec3e2e1c5": {
          "model_module": ***@***.***/controls",
          "model_module_version": "1.1.0",
          "model_name": "VBoxModel",
          "state": {
            "_dom_classes": [
              "widget-interact"
            ],
            "children": [
              "IPY_MODEL_5dc9df01b83045f5b3e93d95b07eccc7",
              "IPY_MODEL_41b984db2c8647a09ec4a7c89033c542"
            ],
            "layout": "IPY_MODEL_ed159285a2204ed18179675c03bc9605"
          }
        },
        "003f9cab73ee4dd4add71f6e1e1b3d50": {
          "model_module": ***@***.***/output",
          "model_module_version": "1.0.0",
          "model_name": "OutputModel",
          "state": {
            "layout": "IPY_MODEL_72632dec96e341329134575d3dc8aaf5"
          }
           ...

And some other notebooks I can see the state is integrated directly,

{
  "kernelspec": {
    "display_name": "Python 3",
    "language": "python",
    "name": "python3"
  },
  "language_info": {
    "codemirror_mode": {
      "name": "ipython",
      "version": 3
    },
    "file_extension": ".py",
    "mimetype": "text/x-python",
    "name": "python",
    "nbconvert_exporter": "python",
    "pygments_lexer": "ipython3",
    "version": "3.6.6"
  },
  "widgets": {
    "state": {
      "e06d5fafe05e421ca7a21d167f5eaa85": {
        "views": [
          {
            "cell_index": 40
          }
        ]
      }
    },
    "version": "1.2.0"
  }
}

So they must have been saved by different versions of jupyter / ipywidgets

Bertrand

On Mon, 14 Feb 2022 at 14:21, Bertrand Nouvel @.***> wrote:

Hi Anton,

You are right that normally the widget state should have created:

The typical cells that were causing issues in the notebook were structured like this:

  • {
  • "data": {
  • "application/vnd.jupyter.widget-view+json": {
  • "model_id": "509796ddf1dd47c19545c4eac2da15e9",
  • "version_major": 2,
  • "version_minor": 0
  • },
  • "text/html": [
  • "

    Failed to display Jupyter Widget of type HBox.\n",

  • "

    \n",

  • " If you're reading this message in the Jupyter Notebook or JupyterLab Notebook, it may mean\n",
  • " that the widgets JavaScript is still loading. If this message persists, it\n",
  • " likely means that the widgets JavaScript library is either not installed or\n",
  • " not enabled. See the <a href=\" https://ipywidgets.readthedocs.io/en/stable/user_install.html\ ">Jupyter\n",
  • " Widgets Documentation for setup instructions.\n",
  • "\n",

  • "

    \n",

  • " If you're reading this message in another frontend (for example, a static\n",
  • " rendering on GitHub or <a href=\"https://nbviewer.jupyter.org/\ ">NBViewer),\n",
  • " it may mean that your frontend doesn't currently support widgets.\n",

  • "

    \n"

  • ]
  • },

As indicated it suggests that the python part of ipywidgets was installed but not the javascript part when the notebook was initially rendered. This is certainly an issue with the notebook - the issue for jupyter-sphinx is just to make it clear to the user that something is wrong inside of the notebook.

You obviously know the code base much more than I do - you may find a better way to help users to identify the nature of the issue than the one I have suggested. For instance, I was not sure if there was a way to indicate the name of the notebook from where the exception was raised - being able to point at the notebook and to put an exact error message would already help a lot.

Kind regards,

Bertrand

On Sat, 12 Feb 2022 at 14:51, Anton Akhmerov @.***> wrote:

I agree that sphinx generally tends to not abort on data inconsistencies. Here, however, it is not clear to me how the error may occur and whether it isn't an actual bug. I believe that jupyter widgets must always write widget state whenever widgets are present. If that is correct, then this not happening would be an actual bug in the widget library and the way it communicates with the kernel, and shouldn't be treated like a user input problem. Basically something should create a widgets metadata entry in the notebook, but then not write widget state into this metadata.

I don't think the error message in the PR corresponds to the situation I described, and therefore wouldn't help debug what is happening. Can you share more information about the conditions under which the problem occurs?

— Reply to this email directly, view it on GitHub https://github.com/jupyter/jupyter-sphinx/pull/196#issuecomment-1037246860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJPMSPBOAFM5567Y4V6EGO3U2ZXYLANCNFSM5OA22YKQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

akhmerov commented 2 years ago

I believe the lower format was used in ipywidgets < 7.0.0, and so it is incompatible with jupyter-sphinx. Since jupyter-sphinx evaluates notebooks by itself, it seems that an installation that meets its requirements would never encounter a problem, or am I missing something?