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

frontend/extension-config: allow default json files in a .d directory #3116

Merged
merged 7 commits into from Jan 5, 2018

Conversation

Projects
None yet
10 participants
@maartenbreddels
Copy link
Contributor

maartenbreddels commented Dec 8, 2017

Original was in ipython/traitlets#452
It was decided it should move out of traitlets.
This allows for instance a jupyter_notebook_config.d/jupyterlab.json to be placed at install time by pip to avoid users having to run jupyter-serverextension enable. Similar for jupyter-widgets.

Note from Steve:
The intent of this PR is to make it easier for notebook and server extensions to be distributed and consumed by users. By using data_files that populate the appropriate config.d directory, the extension is automatically enabled when installed unless overridden by higher level config. This approach works for both wheels and conda packages.

@blink1073
Copy link
Member

blink1073 left a comment

Thanks for this!

@@ -0,0 +1,103 @@
"""Manager to read and modify config data in JSON files.

This comment has been minimized.

@blink1073

blink1073 Dec 8, 2017

Member

Maybe call this config_manager.py?

This comment has been minimized.

@maartenbreddels

maartenbreddels Dec 8, 2017

Author Contributor

yeah, wasn't sure where to put it also, but wanted to start with a clean dup of the original PR. Shall I keep it in the root dir?

This comment has been minimized.

@blink1073

blink1073 Dec 8, 2017

Member

I think so

read_directory = Bool(True)

def ensure_config_dir_exists(self):
try:

This comment has been minimized.

@blink1073

blink1073 Dec 8, 2017

Member

Mind adding short docstrings for these methods?

@bollwyvl

This comment has been minimized.

Copy link
Contributor

bollwyvl commented Dec 8, 2017

Looking awesome! It will be a fine day for packagers and users when it lands.

Some remaining questions:

  • how might we inform a user who is interested/having trouble about which paths will be used and in what order?
    • Perhaps consider a log.debug('Configuration paths for %s: %s', section_name, '\n'.join(paths)) or something
  • how might a user/packager actually create one of these files?
    • Short of just touching it on disk, I guess a CLI... or at least start setting a precedent with nbextensions if that's all this covers right now:
jupyter nbextension enable --sys-prefix --py ipyvolume --confd ipyvolume.json

maartenbreddels added some commits Dec 8, 2017

@maartenbreddels

This comment has been minimized.

Copy link
Contributor Author

maartenbreddels commented Dec 8, 2017

👍 on the logging (done).
I think package manager just create a file and ship that, I don't think this should be generated from a command line (we document it and people copy from existing projects I think).

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

The above command would Just Work™️ when using a conda build though.

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

As in, that file would be created and picked up as an artifact.

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

Although, it wouldn't help with wheels...

@bollwyvl

This comment has been minimized.

Copy link
Contributor

bollwyvl commented Dec 8, 2017

Although, it wouldn't help with wheels...

Seems like you could just include in package_data?

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

Using data_files.

@bollwyvl

This comment has been minimized.

Copy link
Contributor

bollwyvl commented Dec 8, 2017

Ah, right.

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

Kicked the unrelated failing JS test

@@ -0,0 +1,104 @@
"""Manager to read and modify config data in JSON files.
"""
# Copyright (c) IPython Development Team.

This comment has been minimized.

@damianavila

damianavila Dec 8, 2017

Member

Since you are porting this from traitlets, it shows some oldies... let's use the updated header.

maartenbreddels added some commits Dec 8, 2017

@maartenbreddels

This comment has been minimized.

Copy link
Contributor Author

maartenbreddels commented Dec 8, 2017

I see another PR failing on that part, so I guess that is not related.

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 8, 2017

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Dec 15, 2017

Unrelated errors fixed, this should be good to go.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Dec 19, 2017

This looks great! Going forward, where extension authors want to enable extensions just by creating a new file (the goal of removing post-link scripts in conda packages), what would an entry point for that be? An extra arg to nbextension enable that writes a new file rather than editing the existing one?

@maartenbreddels

This comment has been minimized.

Copy link
Contributor Author

maartenbreddels commented Dec 19, 2017

My feeling about this is that since it's for package creators and meant to put in place using setuptools' data_files, that it's fine that it is just documented somewhere how to do this (a TODO). Automating this doesn't add much value I think.

@bollwyvl

This comment has been minimized.

Copy link
Contributor

bollwyvl commented Dec 21, 2017

Re: automation: perhaps it could be reasonable to create/update an existing cookiecutter to reflect how this can be used. I think the jupyterlab/*-cookiecutters have been useful in that regard!

@maartenbreddels

This comment has been minimized.

Copy link
Contributor Author

maartenbreddels commented Dec 21, 2017

@stonebig

This comment has been minimized.

Copy link

stonebig commented Jan 5, 2018

so I don't need to do anymore things like "jupyter.exe labextension install bqplot" (with coming beta) ?

in my process I install wheels in an undefined order. Must I put Jupyterlab before anything else to get it automated ?

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Jan 5, 2018

Correct, but you would still need nodejs installed for the build portion. If you didn't build on your own, it would prompt you to build when JupyterLab launches, and then prompt you to reload when the build succeeds.

@stonebig

This comment has been minimized.

Copy link

stonebig commented Jan 5, 2018

the build portion ? pip install won't trigger a nodejs thing, will it ?

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Jan 5, 2018

No, but when JupyterLab launches it checks for installed extensions that are not part of the JupyterLab bundle.

@stonebig

This comment has been minimized.

Copy link

stonebig commented Jan 5, 2018

ah, so if I want that to happen, I need to launch jupyterlab (in headless/useless mode) once ... or to keep these "labextension" install.

is there a simple way to launch jupyterlab just for that ? a sort of "jupyterlab --check_your_extensions_and_stop" ?

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Jan 5, 2018

You can run jupyter lab build at any time, no need to launch JupyterLab. It is a user convenience to auto-build if necessary.

@blink1073 blink1073 referenced this pull request Jan 5, 2018

Merged

Add data_files support #3546

@blink1073

This comment has been minimized.

Copy link
Member

blink1073 commented Jan 5, 2018

@SylvainCorlay

This comment has been minimized.

Copy link
Member

SylvainCorlay commented Jan 5, 2018

This is really cool, thanks @maartenbreddels @blink1073 .

@rgbkrk

This comment has been minimized.

Copy link
Member

rgbkrk commented Jan 5, 2018

Thanks for this!

@jasongrout

This comment has been minimized.

Copy link
Member

jasongrout commented Jan 5, 2018

Thank you @maartenbreddels! I think this will solve a lot of the confusion around getting widgets working.

vidartf added a commit to jupyter-widgets/widget-ts-cookiecutter that referenced this pull request Jan 9, 2018

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 10, 2018

So I really like this idea. I wish I'd realised this was part of the release sooner, because I really don't think this should be in the notebook library.

I actually am concerned that this is in notebook and not inside jupyter_core.paths, where the rest of the configuration finding mechanisms live, see this function as a partial reference:

https://github.com/jupyter/jupyter_core/blob/master/jupyter_core/paths.py#L180

Frequently, users, both experienced and inexperienced, have ended up in weird states with their config files as it is. The worst part about it is that its really hard to debug because there are so many places config files could live; as expected, these kinds of bugs hit more novice users the worst. That's why I released jupyter_conf_search. What made that utility possible was that there is a single canonical mechanism for finding your configuration files and the order in which they are loaded.

I'm worried that this is going to make these kinds of configuration bugs even harder to debug.

@gnestor Is it possible to remove this from the notebook release and put it inside jupyter core instead? I don't see why it makes more sense here given that it seems to be useful for the entire jupyter stack. edit see #3116 (comment)

@jasongrout

This comment has been minimized.

Copy link
Member

jasongrout commented Jan 11, 2018

@mpacer - to raise the visibility of your objections, can you:

  1. open a new issue under the 5.3 milestone raising your objection, and
  2. note in the 5.3 release issue your objection and reference your issue: #3183

I'm afraid having a comment here on a closed issue will be less visible.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jan 11, 2018

Will follow-up in the new issue when it’s open, but this is only applied to the very notebook-specific configuration of extensions, not the rest of “normal” traitlets-based configuration that’s shared across packages. You are right that adding .d-style config to that belongs in a higher-level for Jupyter-core or traitlets (see my attempt: ipython/traitlets#242). The fact that extensions use a totally different configuration system is an issue and source of confusion, but not a new one. Adding conf.d to traitlets/core also wouldn’t solve the issue for extensions, because it’s a different setup, so we would have to do it twice anyway. The core.paths are still relevant, this is an additional layer inside a given directory on those paths.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 11, 2018

@minrk thanks for the clarification. My concern was because I didn't understand how the class was being used. In particular, seeing the default value of config_dir=Unicode('.') made my twitch, since that would default to not using the jupyter config system. In all the cases it's used though, it's either inside tests or we're using the juptyer config locations, so my primary concerns about this being a release blocking are moot.

The urgency stemmed from a fear of having to explain to people how to deal with a whole new set of locations that config files might exist in. That isn't the case here, so s'all good for now. I'll open an issue over the next few days in either jupyter_core or traitlets where I can non-urgently express this concern.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jan 11, 2018

Ah, good point. The default value is only because it's technically a generic object. I suspect the default is never used, and a note to that effect might be appropriate. This PR doesn't change where files are looked for (jupyter-paths is still respected), only that wherever we loaded [jupyter-path]/nbconfig/tree.json we can now also load [jupyter-path]/nbconfig/tree.d/*.json. A similar proposal is ongoing in traitlets to get notebook_config.d wherever we already find notebook_config.{py|json}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.