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

Bundler extension API #1579

Merged
merged 15 commits into from
Aug 13, 2016
Merged

Bundler extension API #1579

merged 15 commits into from
Aug 13, 2016

Conversation

parente
Copy link
Member

@parente parente commented Jun 30, 2016

For #1469

  • port existing code from jupyter_cms
  • switching to using content manager instead of absolute file paths
  • make API resources consistent with /nbconvert
  • make menu item ordering consistent
  • add cli option for listing bundles
  • add zip bundler as example
  • port bundler tests
  • add bundler extension management tests
  • port documentation
  • decide if all bundler tools should remain here (I think yes, as examples, along with zip_bundler.py showing how to use them)

@minrk minrk added this to the 5.0 milestone Jun 30, 2016
@@ -0,0 +1,259 @@
# Copyright (c) Jupyter Development Team.
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to put this file inside notebook/bundler so it's all self contained.

* Change handler for consistency with nbconvert
* Change bundler function API to take ContentManager models
* Change CLI to work as jupyter bundler
* Change UI to fit into existing template / JS structure

(c) Copyright IBM Corp. 2016
Based on first review by @minrk

* bundlerextension for consistency
* Move CLI entry point into bundler package
* Add bundler/__main__.py

(c) Copyright IBM Corp. 2016
(c) Copyright IBM Corp. 2016
@parente
Copy link
Member Author

parente commented Jul 2, 2016

@minrk Do you think this is reasonable to target for 4.3 instead of 5.0? There's only new API, no backward incompatibility. It would help us move the dashboard bundlers away from their dependency on another extension (jupyter_cms) sooner rather than later.

parente added 8 commits July 2, 2016 23:15
Cleanup more bundler vs bundlerextension refs

(c) Copyright IBM Corp. 2016
(c) Copyright IBM Corp. 2016
(c) Copyright IBM Corp. 2016
It has no instance state. It should be a module that bundlers optionally import.

(c) Copyright IBM Corp. 2016
(c) Copyright IBM Corp. 2016
(c) Copyright IBM Corp. 2016
(c) Copyright IBM Corp. 2016
(c) Copyright IBM Corp. 2016
@minrk
Copy link
Member

minrk commented Jul 4, 2016

It could be for 4.3, if we do that. We may be at the point of only doing bugfix backports for 4.x at this point, though.

parente added 2 commits July 4, 2016 17:34
Include label and module name (if available) to
help a user remember which Python module provides
which bundler(s)

(c) Copyright IBM Corp. 2016
In notebook.json config

(c) Copyright IBM Corp. 2016
@parente
Copy link
Member Author

parente commented Jul 5, 2016

For the docs, I'm thinking of adding a new docs/source/extending/bundlerextensions.rst and expanding the content in docs/source/examples/Notebook/Distributing Jupyter Extensions as Python Packages.ipynb to also mention what needs to be done to distribute bundler extensions.

Does this sound reasonable?

/cc @willingc

@minrk
Copy link
Member

minrk commented Jul 5, 2016

Sounds good to me

parente added 2 commits July 5, 2016 13:12
* Improve some docstrings too
* Fix a busted extension link along the way

(c) Copyright IBM Corp. 2016
* Add tarball_bundler example inline
* Add tarball_bundler example to bundler doc too
* Cross-ref from distributing doc to doc about writing extensions

(c) Copyright IBM Corp. 2016
@parente parente changed the title [wip] Bundler API Bundler extension API Jul 5, 2016
@parente
Copy link
Member Author

parente commented Jul 5, 2016

I've checked off everything I had planned on my list here. There's examples, tests, doc, and the implementation itself.

@willingc
Copy link
Member

willingc commented Jul 5, 2016

@parente Looks good to me. Thanks!

@parente
Copy link
Member Author

parente commented Jul 7, 2016

I think this is ready for a final review / merge.

Adding @Carreau and @ellisonbg who commented on the original associated issue and cc'ed @fperez @jasongrout.

@torsstei
Copy link

It would be desirable to move this pull request towards merge soon. We have an exploitation of the cms (https://www.youtube.com/watch?v=xSRZgxAf9BQ) and a regular notebook core API would be what we would prefer.

@gnestor
Copy link
Contributor

gnestor commented Aug 11, 2016

@parente I'll review this. A couple questions:

  • How can I get the bundlerextension command working? I pulled this PR branch and re-installed using pip install -e . --user...
  • Once the bundlerextension extension is working, can I test the zip bundler using jupyter bundlerextension enable --py notebook.bundler.zip_bundler --sys-prefix?
  • Anything else I should test?

@parente
Copy link
Member Author

parente commented Aug 12, 2016

@gnestor Thanks for having a look!

How can I get the bundlerextension command working? I pulled this PR branch and re-installed using pip install -e . --user...

jupyter bundlerextension should just work. Are you saying it doesn't? I dev'ed locally by pip installing into a conda environment. Is --user breaking it?

Once the bundlerextension extension is working, can I test the zip bundler using jupyter bundlerextension enable --py notebook.bundler.zip_bundler --sys-prefix?

This is definitely a good place to start. There's a tarball bundler in there too that you can try also.

Anything else I should test?

jupyter/dashboards_bundlers#46 makes the dashboards bundlers extension compatible with this new API. If you're feeling brave, you might try testing that.It'll take a bit of doing, namely getting the dashboard extension installed, the bundler extension built with the PR installed, and the dashboard server running to see if the bundler properly packs and sends one of the example notebook-dashboards to the dashboard server properly. If that's too much overhead (and it probably is), don't sweat it. I did test it once already when creating that PR. If this merges in, I'll test it against notebook HEAD.

@gnestor
Copy link
Contributor

gnestor commented Aug 12, 2016

jupyter bundlerextension should just work. Are you saying it doesn't? I dev'ed locally by pip installing into a conda environment. Is --user breaking it?

Yep, the --user flag was the culprit.

zip and tarball bundlers worked for me! It took about 20-30 seconds for the first zip to fulfill. Each subsequent one was instant, along with the tarball.

@gnestor
Copy link
Contributor

gnestor commented Aug 12, 2016

@minrk How do you feel about merging this?

@minrk
Copy link
Member

minrk commented Aug 13, 2016

@gnestor 👍

@minrk minrk merged commit f396334 into jupyter:master Aug 13, 2016
@parente
Copy link
Member Author

parente commented Aug 13, 2016

@minrk @gnestor Not sure why --user didn't work. I can look into that as follow-on. Thanks for testing and merging!

@gnestor
Copy link
Contributor

gnestor commented Aug 13, 2016

@parente Ya please do because it could be my environment...

@parente
Copy link
Member Author

parente commented Aug 23, 2016

@gnestor I pulled master just now and ran:

cd notebook
source activate notebook-dev
pip install -e .
jupyter bundlerextension enable --py notebook.bundler.zip_bundler --user
jupyter notebook

Then I opened a notebook and triggered the bundler menu item. No issues.

I next tried the following outside a conda env:

cd notebook
pip install -e . --user
~/.local/bin/jupyter bundlerextension enable --py notebook.bundler.zip_bundler --user
~/.local/bin/jupyter notebook

No issues again.

@gnestor
Copy link
Contributor

gnestor commented Aug 24, 2016

@parente Ok good. Great work, this is an exciting addition! Any bundle as Gist bundlers yet? If not, that could be my first bundler contribution...

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

Successfully merging this pull request may close these issues.

5 participants