Skip to content
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

colab compatibility #77

Closed
wants to merge 15 commits into from
Closed

Conversation

kushalkolar
Copy link
Contributor

@kushalkolar kushalkolar commented Jul 17, 2023

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
Copy link
Member

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
Copy link
Contributor Author

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 kushalkolar marked this pull request as ready for review September 23, 2023 02:16
Copy link
Member

@almarklein almarklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see this converge! Thanks!

.github/workflows/publish.yml Outdated Show resolved Hide resolved
.github/workflows/publish.yml Show resolved Hide resolved
Copy link
Contributor

@manzt manzt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@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.

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.

jupyter_rfb/_colab.py Show resolved Hide resolved
jupyter_rfb/_colab.py Outdated Show resolved Hide resolved
jupyter_rfb/_colab.py Show resolved Hide resolved
@kushalkolar
Copy link
Contributor Author

@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 😄 . Thanks again for figuring this out!

@manzt
Copy link
Contributor

manzt commented Sep 26, 2023

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
Copy link
Contributor

manzt commented Sep 26, 2023

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
Copy link
Member

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
Copy link
Contributor

manzt commented Sep 28, 2023

@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
Copy link
Contributor

manzt commented Sep 28, 2023

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.

- name: publish npm
run: |
cd js
npm publish
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The js seems to use the yarn package manager, not npm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@kushalkolar kushalkolar Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manzt Are yarn published packages still findable by colab?

jupyter_rfb/_colab.py Outdated Show resolved Hide resolved
jupyter_rfb/_colab.py Outdated Show resolved Hide resolved
jupyter_rfb/_colab.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@kushalkolar kushalkolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ready for another look!

- name: publish npm
run: |
cd js
npm publish
Copy link
Contributor Author

@kushalkolar kushalkolar Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manzt Are yarn published packages still findable by colab?

jupyter_rfb/_colab.py Outdated Show resolved Hide resolved
@almarklein
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

manzt commented Oct 25, 2023

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
Copy link
Contributor Author

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/[email protected]/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!

colab-slow-2023-10-28_21.45.32.mp4

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

we can probably get this working in collab
3 participants