-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
New nbextensions installation API #879
Conversation
@@ -187,6 +187,7 @@ def init_settings(self, ipython_app, kernel_manager, contents_manager, | |||
}, | |||
version_hash=version_hash, | |||
ignore_minified_js=ipython_app.ignore_minified_js, | |||
nbextensions=ipython_app.nbextensions, |
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.
We need to be smarter if we want this to be updated while the server runs.
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.
Again, I want to keep it simple. I think that allowing updating this while the server runs is out of scope. But, there is nothing to prevent someone else from just calling utils.load_extensions()
in the frontend at any time to load an extension. We could even provide an IPython magic that does that, or another extension could be written to handle runtime loading.
@@ -645,6 +647,12 @@ def nbextensions_path(self): | |||
path.append(os.path.join(get_ipython_dir(), 'nbextensions')) | |||
return path | |||
|
|||
nbextensions_tree = Dict(config=True, | |||
help="A list of nbextensions to enable for the tree/dashboard page.") |
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.
s/list/dict(key:bool)/
OK, the current state of this code allows you to install the following nbextensions: https://github.com/jdfreder/jupyter-tree-filter Using the following sequence of commands (notebook doesn't have to be restarted):
I am going to work on cleaning it all up, but review can start. |
My diff comment got clobbered in an update, so reposting as a comment to make sure it's not lost: there's a third view, the It means adding one more if/else case in a few spots. |
"""Config Manager used for storing notebook frontend config""" | ||
|
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.
Ref for #893 discussion
Hopefully, I'm not late to any reviews but I just pulled this PR locally and tested out what I could. I noticed that that prior to rewrite Edit: Also not sure if now would be the right time to start writing some documentation on this, if so, let me know I can get started and we can build documentation/code in parallel. Edit 2: I have some questions/comments about logging but will wait until you are done with that part of the PR. |
@@ -292,7 +292,7 @@ def check_origin(self, origin_to_satisfy_tornado=""): | |||
#--------------------------------------------------------------- | |||
# template rendering | |||
#--------------------------------------------------------------- | |||
|
|||
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.
OCD kicking in, but might want to remove this whitespace.
|
||
def _read_config_data(user=False, sys_prefix=False): | ||
config_dir = _get_config_dir(user=user, sys_prefix=sys_prefix) | ||
config_file = os.path.join(config_dir, 'jupyter_notebook_config.json') |
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.
it would be nice if jupyter_notebook_config.json
was exposed as a constant someplace: but hopefully nobody should ever have to deal with this again!
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.
ConfigManager can be used to do this reading/writing, without any open/json/ensure_dir calls:
config_man = BaseJSONConfigManager(config_dir)
config = config_man.get('jupyter_notebook_config')
# < update config >
config_man.update('jupyter_notebook_config', new_config)
prefix=self.prefix, | ||
destination=self.destination, | ||
nbextensions_dir=self.nbextensions_dir, | ||
if self.python: |
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.
these duplicate blocks can be removed by making the function a variable:
install = install_nbextension_python if self.python else install_nbextension
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.
✔️
@ellisonbg I worked on installing this branch and trying to migrate the https://github.com/jupyter-incubator/contentmanagement extension to using this to install/enable instead of its own custom CLI (https://github.com/jupyter-incubator/contentmanagement/blob/master/jupyter_cms/extensionapp.py). Here's what I found:
To double-check, I tried nbgrader's CLI. It, too, raises an error because the base class nbextension implementation has changed.
Current takeaways:
Small update: Here's the metadata I put into def _jupyter_nbextension_paths():
return [{
'section': 'notebook',
'src': 'nbextension',
'dest': 'jupyter_cms',
'require': 'jupyter_cms/notebook/main'
},
{
'section': 'tree',
'src': 'nbextension',
'dest': 'jupyter_cms',
'require': 'jupyter_cms/dashboard/main'
},
# below does not get picked up
{
'section': 'edit',
'src': 'nbextension',
'dest': 'jupyter_cms',
'require': 'jupyter_cms/editor/main'
}] |
This is really important... otherwise people working with nbextension/serverextension complex extensions should implement an additional CLI as @parente mentioned or alternative a custom configmanager to write config outside the user space (since the current config manager implementation on this PR only writes to the user space) What about adding the server extension info at the python metadata level? I mean adding a field to load that info if it is provided... In that way we should only modify I mean: def _jupyter_nbextension_paths():
return [{
'section': 'notebook',
'src': 'amd',
'dest': 'jupyter-lightsaber',
'require': 'jupyter-lightsaber/index',
'server': ['my_server_ext1', 'my_server_ext2']
}] and then make
Not sure if it will be a nice way to not break this since the config manager implementation has changed a lot on this PR... version detection seems like a good alternative. |
Additionally, the CLI should offer some keyword to enable/disable the server part... I guess... or maybe an independent CLI for the server part... |
I would probably treat server and client extensions separately |
@@ -299,118 +355,300 @@ def _config_file_name_default(self): | |||
def install_extensions(self): | |||
if len(self.extra_args)>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.
this is triggering an error when I tried to install using --py in a sys-prefix... I mean:
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.
removing this condition work as expected... if we want to warn more than one nbextension we should detect that in another way...
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.
Forget about this... it is working...
Ok, with that PR in... are we looking for anything else? More doc? Glad to help! |
logging is inappropriate here also only show config dir if there's config in it.
enable is user by default
add more docs coverage, help output So that the three cases are covered: - system-wide (default for install) - user (default for enable) - sys-prefix
@bollwyvl I think we're almost there. We can merge soon and make the beta. I opened ellisonbg#24 with a few things I found that were still missing. |
@bollwyvl If you think that there are more docs needed, please dive in or ping me. Thanks :D |
updates to nbextensions
I'm happy to merge this as-is, now. |
I'm about to board my flight to California, but I'll merge by the end of Friday (CA time) unless someone wants to do anything else here. |
Safe travels @minrk. |
@minrk safe travels |
🎉 🍰 Welcome back to cali @minrk (and @takluyver) ! https://www.youtube.com/watch?v=E_B0rOLvAOo :) |
Thanks Jon! See you on Monday (or let me know if you're getting to the bay
area sooner.
https://youtu.be/5NTqZ347TKY
|
Welcome back @takluyver. See you on Monday. Congrats to all on getting this merged. |
🍰 👍 |
🍦 🍰 🍕 from DFW... See y'all soon! On 09:07, Sun, Mar 27, 2016 Sylvain Corlay [email protected] wrote:
|
\o/ |
This is working draft exploring the feasibility of the first part of the work described in #878
So far this does:
nbextensions
config traitlet to the mainNotebookApp
.main.js
could access it.