-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Enable WIT usage in JupyterLab/Cloud AI Platform notebooks #2301
Conversation
@tolga-b please review |
Install and enable WIT for JupyterLab by running a cell containing: | ||
``` | ||
!pip install witwidget | ||
!jupyter labextension install wit-widget |
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.
Check out npm postinstall scripts, it might be able to do these steps. (but your script will need to be compatible with all the environments it could run—or check which environment it is in).
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.
i'm going to leave this as-is for now, as in some environments it may need to be done with sudo (cloud ai platform notebooks), but not in your own jupyterlab instance.
@@ -13,6 +13,7 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
==============================================================================*/ | |||
|
|||
var witHtmlLocation = require('file-loader!./wit_jupyter.html'); |
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.
I'd double check to see if you need the file-loader!
part. Given your webpack config i wouldn't think that you would need it.
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 jupyter labextension install fails if i leave it out:
ERROR in ./node_modules/wit-widget/lib/wit_jupyter.html 1:0
Module parse failed: Unexpected token (1:0)
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.
Agreed with @tafsiri—perhaps you need to add module: {rules: rules}
to
the notebook extension config, as in the other two configs?
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.
i tried that as well - no luck :(
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.
Hmm, okay; strange, but I’ll let it be.
|
||
// Adjust WIT html location if running in a jupyter notebook | ||
// and not in jupyterlab. | ||
if (document.querySelector('body').getAttribute('data-base-url') != null) { |
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.
do you know what the value of data-base-url would be? would that make it a more robust check? I admit this is a hard thing to test. Is there a version string somewhere that might have the project name or something.
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.
I believe it may depend on your own Jupyter installation, so I'm not looking to test a specific value of that field, just that it exists
// Adjust WIT html location if running in a jupyter notebook | ||
// and not in jupyterlab. | ||
if (document.querySelector('body').getAttribute('data-base-url') != null) { | ||
witHtmlLocation = window.__webpack_public_path__ + 'wit_jupyter.html'; |
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.
I'd recommend renaming __webpack_public_path__
to something like __nbextension_path__
.
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.
done
@@ -16,8 +14,8 @@ | |||
"jupyterlab-extension" | |||
], | |||
"files": [ | |||
"lib/**/*.js", | |||
"dist/*.js" | |||
"lib/*", |
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.
totally up to you, but longterm might be nice to have both of them use dist? (Though there may be valid reasons to have them use diff sources/builds)
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.
done, removed lib
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.
was actually mistaken about this - labextensions fail to install unless the npm package contains lib/labplugin.js - added it back in
"@jupyter-widgets/base": "^1.0.0" | ||
"@jupyter-widgets/base": "^1.0.0", | ||
"@jupyter-widgets/jupyterlab-manager": "^0.38.1", | ||
"file-loader": "^3.0.1" |
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.
I think this can go in devDependencies
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.
done
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.
LGTM
!jupyter labextension install wit-widget | ||
!jupyter labextension install @jupyter-widgets/jupyterlab-manager | ||
``` | ||
For TensorFlow GPU support, use the witwidget-gpu package instead of witwidget. |
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.
nit: Code spans around witwidget-gpu
, witwidget
, and the !sudo
command on the next line.
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.
done
@@ -13,6 +13,7 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
==============================================================================*/ | |||
|
|||
var witHtmlLocation = require('file-loader!./wit_jupyter.html'); |
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.
Agreed with @tafsiri—perhaps you need to add module: {rules: rules}
to
the notebook extension config, as in the other two configs?
|
||
// Adjust WIT html location if running in a jupyter notebook | ||
// and not in jupyterlab. | ||
if (document.querySelector('body').getAttribute('data-base-url') != null) { |
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.
Superfluous querySelector
(just use document.body
). If you like, you
can also use document.body.dataset.baseUrl
instead of getAttribute
,
but up to you.
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.
changed to document.body
witHtmlLocation = window.__nbextension_path__ + 'wit_jupyter.html'; | ||
} | ||
|
||
const src = `<link rel="import" href="${witHtmlLocation}"> |
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.
Please note that this will break in Chrome M75, which is scheduled to
be released… uh, today. Maybe test with Chrome beta/canary?
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.
thanks. will handle this in separate PR ASAP
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.
Sounds good; 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.
I tested WIT in colab in chrome canary (which is running 77.0.3815.0) and it works as currently implemented.
at https://packaging.python.org/tutorials/packaging-projects/#uploading-the-distribution-archives. | ||
4. Commit the version.py change. | ||
5. Publish the new NPM package through running "npm publish" in the "js/" subdirectory of the package files generated in step 3. |
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.
nit: Code spans: `npm publish`
; `js/`
.
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.
done
at https://packaging.python.org/tutorials/packaging-projects/#uploading-the-distribution-archives. | ||
4. Commit the version.py change. | ||
5. Publish the new NPM package through running "npm publish" in the "js/" subdirectory of the package files generated in step 3. | ||
6. Commit the version changes. |
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.
Shouldn’t the process be to commit and merge any version changes
before publishing to PyPI and NPM, then always publish from a
checked-in, known-good commit? Publishing immutable archives from a
dirty work tree doesn’t sound like something that we want to encourage.
Not only are you vulnerable to typos or bad local state, but others
might not be able to reliably reproduce the built artifacts. It’s much
simpler if the actual deployment process is just “clone fresh; check out
some fixed commit; run npm publish
, et al.”.
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.
sounds better. updated instructions.
@@ -13,6 +13,7 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
==============================================================================*/ | |||
|
|||
var witHtmlLocation = require('file-loader!./wit_jupyter.html'); |
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.
Hmm, okay; strange, but I’ll let it be.
witHtmlLocation = window.__nbextension_path__ + 'wit_jupyter.html'; | ||
} | ||
|
||
const src = `<link rel="import" href="${witHtmlLocation}"> |
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.
Sounds good; thanks!
Update WitWidget packaging/logic to enable it to work in JupyterLab notebooks, which handle extensions like WitWidget differently than standard Jupyter notebooks. This also enables use in Cloud AI Platform notebooks.
Update packaging code to create correct, public NPM package, as the HTML/JS side of lab extensions are NPM packages.
Update witwidget jupyter js code to handle loading wit_jupyter.html from correct packaged location when running in JupyterLab, while keeping the same initial logic for loading that file when in standard Jupyter, as they are different paths.
Update package building script and webpack config to correctly bundle wit_jupyter.html with the NPM package for the JupyterLab extension.
Update documentation.
No visual changes.
Installed/ran WIT in Cloud AI Platform notebooks (which are JupyterLab notebooks).
Installed/ran WIT in standard Jupyter notebooks to ensure this change didn't break that case.