-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
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. |
Changed from conditional imports to @manzt, does the publish workflow look ok to you? Is that all that's required for |
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.
Nice to see this converge! Thanks!
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.
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.
Is there a way to check these things before or without running the github action?
I guess anywidget would be a more longterm solution, what do you think? @almarklein
No worries, I've been busy as well 😄 . Thanks again for figuring this out! |
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.
Maybe something to think about if targeting multiple platforms becomes unwieldy. |
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 |
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? |
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 I can try to have a look, but I can't promise a timeline. |
Ah, having a look. It might be a more substantial refactor than I'd imagined due to how the |
.github/workflows/publish.yml
Outdated
- name: publish npm | ||
run: | | ||
cd js | ||
npm publish |
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.
The js
seems to use the yarn
package manager, not npm
.
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.
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.
@manzt Are yarn published packages still findable by colab?
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.
ready for another look!
.github/workflows/publish.yml
Outdated
- name: publish npm | ||
run: | | ||
cd js | ||
npm publish |
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.
@manzt Are yarn published packages still findable by colab?
Co-authored-by: Trevor Manz <[email protected]>
Co-authored-by: Almar Klein <[email protected]>
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 |
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
|
Did some more work on this by testing locally and publishing to npm manually:
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.mp4The bandwidth is fast, |
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. |
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.