colab compatibility - Githubissues

kushalkolar commented 11 months ago

closes #74

RemoteFrameBuffer._repr_mimebundle_ returns (data, colab_metadata) when running in colab. Also adds github action to publish to pypi and npm. Publishing to npm is required for running in colab since that's where it fetches widget stuff from.

almarklein commented 11 months ago

Conditional imports can be a pain for tools like PyInstaller, so I'd rather avoid them 😅. I think that by creating a function that optionally provides extra metadata, like you already do, we avoid polluting the main widget class, and we can also extend it easily for other clients if needed.

kushalkolar commented 9 months ago

Changed from conditional imports to try-except imports for colab.

@manzt, does the publish workflow look ok to you? Is that all that's required for npm publish?

kushalkolar commented 9 months ago

@manzt, does the publish workflow look ok to you? Is that all that's required for npm publish?

Yeah, this mostly looks correct to me. There are so many gotchas with publishing to NPM that it's hard to know that 1.) It is packaged correctly (i.e., webpack config) and 2.) that it is discoverable on NPM by the CDN manager used in Colab.

Is there a way to check these things before or without running the github action?

Shameless plug: anywidget takes care of all of these issues (and more), but would require refactoring jupyter_rfb. It's a trade-off between managing build configurations or refactor.

I guess anywidget would be a more longterm solution, what do you think? @almarklein

Sorry it took a while for me to review. The idea is correct, but I think there are some issues in the implementation which will lead to the incorrect behavior in Colab.

No worries, I've been busy as well :smile: . Thanks again for figuring this out!

manzt commented 9 months ago

Is there a way to check these things before or without running the github action?

Not that I'm aware of. That's kind of why the Jupyter Widgets ecosystem is so tough. The widget template repositories contain solutions, but it's not explicit how these things are linked – just lots of build implicitly linked build configurations between javascript and Python. Colab in particular is very difficult to test against because it imports dependencies from a CDN (like https://unpkg.com/), which is a production environment. I guess maybe you could try to proxy requests but I haven't done that myself.

I guess anywidget would be a more longterm solution, what do you think? @almarklein

Maybe something to think about if targeting multiple platforms becomes unwieldy.

manzt commented 9 months ago

Looking closer, I'm 90% sure to publish this correctly you need to add:

{
  "name": "jupyter_rfb",
  "version": "0.1.0",
  "description": "Remote Frame Buffer for Jupyter",
  "author": "Almar Klein",
  "main": "lib/index.js",
++ "publishConfig": {
++   "main": "dist/index.js"
++ },
  "repository": {
    "type": "git",
    "url": "https://github.com/vispy/jupyter_rfb.git"
  },
  "keywords": [
    "jupyter",
    "widgets",
    "ipython",
    "ipywidgets",
    "jupyterlab-extension"
  ],
  "files": [
    "lib/**/*.js",
    "dist/*.js"
  ],
  "scripts": {
    "clean": "rimraf dist/ && rimraf ../jupyter_rfb/labextension/ && rimraf ../jupyter_rfb/nbextension",
    "prepublish": "yarn run clean && yarn run build:prod",
    "build": "webpack --mode=development && yarn run build:labextension:dev",
    "build:prod": "webpack --mode=production && yarn run build:labextension",
    "build:labextension": "jupyter labextension build .",
    "build:labextension:dev": "jupyter labextension build --development True .",
    "watch": "webpack --watch --mode=development",
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "devDependencies": {
    "@jupyterlab/builder": "^3.0.0",
    "webpack": "^5",
    "rimraf": "^2.6.1"
  },
  "dependencies": {
    "@jupyter-widgets/base": "^1.1 || ^2 || ^3 || ^4 || ^5 || ^6",
    "lodash": "^4.17.4"
  },
  "jupyterlab": {
    "extension": "lib/labplugin",
    "outputDir": "../jupyter_rfb/labextension",
    "sharedPackages": {
      "@jupyter-widgets/base": {
        "bundled": false,
        "singleton": true
      }
    }
  }
}

Normally, you'd just put dist/index.js in the "main" field for an NPM package, but jupyter labextension build needs this to be src/index.js to build the extension for JupyterLab. The publishConfig lets you override fields when you publish to NPM, so the main field will be dist/index.js when your publish and will allow the CDN to discover the entry point.

almarklein commented 9 months ago

I guess anywidget would be a more longterm solution, what do you think? @almarklein

Well, if it makes targeting other colab platforms more easy, perhaps it's worth looking into it now. @manzt you mentioned we'd need to refactor some things. I assume most of the logic can stay in place, but it'd be mostly a matter of connecting things up?

manzt commented 9 months ago

@manzt you mentioned we'd need to refactor some things. I assume most of the logic can stay in place, but it'd be mostly a matter of connecting things up?

I haven't had the chance to look myself. But in theory things should mostly be a matter of connecting things. The scale of the refactor generally depends on how much you extend the DOMWidgetModel (or rely on backbone-specific things) because we've tried to narrow the API surface area from jupyter-widgets.

I can try to have a look, but I can't promise a timeline.

manzt commented 9 months ago

Ah, having a look. It might be a more substantial refactor than I'd imagined due to how the RemoteFrameBufferModel is constructed and communicates to the views. I'm actually not sure how this would be done currently in anywidget. Certainly something one could implement, but perhaps we can try to come up with a more reusable solution for this pattern.

almarklein commented 8 months ago

Just a gentle ping.

IIUC this PR is nearly there, right? If so, I propose to wrap this up and look at the anywidget solution another time.

kushalkolar commented 8 months ago

Just a gentle ping.

IIUC this PR is nearly there, right? If so, I propose to wrap this up and look at the anywidget solution another time.

Yes! Just waiting for @manzt to give the go-ahead on the CI and whether yarn publish as opposed to npm publish still makes it findable by colab.

manzt commented 8 months ago

yarn publish as opposed to npm publish still makes it findable by colab.

The choice of package manager shouldn't matter too much. You just want to make sure the tarball generated to be published to the npm registry includes dist/index.js. Should be able to test locally with:

cd js && yarn pack
tar -xzf <tarball>

kushalkolar commented 8 months ago

Did some more work on this by testing locally and publishing to npm manually:

Even after modifying the version strings in js/package.json and js/lib/widget.js, it was still looking for index.js with v0.1.0, i.e. it was looking for https://cdn.jsdelivr.net/npm/jupyter_rfb@0.1.0/dist/index.js. I'm not sure what else has to be modified to make it look for the right version.
I did a manual publish by keeping it at v0.1.0 to test things, the examples work. The resize handler isn't present.

The performance is extremely slow which is starting to make me think if we really want to even merge this. I'm getting 2fps!

https://github.com/vispy/jupyter_rfb/assets/9403332/6a008594-187c-40e6-8996-7a1a12738968

The bandwidth is fast, speedtest-cli from within colab gives very high upload rates, so there must be some other bottleneck. I think it might be limited the limited CPU power for jpeg encoding. We could document it and point users to other platforms if they want the full performance.

kushalkolar commented 4 months ago

I'm going to close this, the performance is very poor and not worth the maintenance burden. I would suggest users use jupyterhub, binder, or vscode notebooks for teaching purposes.

vispy / jupyter_rfb

colab compatibility #77