allow default json files in a .d directory #452

maartenbreddels · 2017-09-01T13:18:36Z

This allows to have in addition to a single foobar.json file a foobar.d directory, where all '*.json' files will be read, and used as default values.
Would make implementing jupyter/notebook#2824 trivial.

minrk · 2017-11-14T09:52:24Z

traitlets/config/manager.py

+                for path in sorted(paths):
+                    name, ext = os.path.splitext(path)
+                    abspath = os.path.join(directory, path)
+                    if ext == ".json" and name not in data:


Why the name not in data here? Shouldn't all files be loaded?

No need for that, leftover from refactoring the code. The idea is that the .json file overrride any values that are set in the .d directory files, but instead I just swapped the order (first read the directory, then read the .json).

Perhaps could do a discovery phase and then a load phase, and let bad configs not cause the whole stack to fail, a la:

from glob import glob ... def get(self, section_name): """Retrieve the config data for the specified section. Returns the data as a dictionary, or an empty dictionary if the file doesn't exist. """ paths = [self.file_name(section_name)] if self.read_directory: pattern = os.path.join(self.directory(section_name), '*.json') paths = sorted(glob(pattern)) + paths data = {} for path in paths: try: if os.path.isfile(path): with io.open(path, encoding='utf-8') as f: recursive_update(data, json.load(f)) except Exception as err: self.log.error( 'Couldn\'t update %s configuration from %s: %s', section_name, path, err) return data

Yup, good idea. I think this is an artifact from a misunderstanding, i thought the 'dict' foo.d/bar.json would to into data['bar']. I'll update it.
Not sure if it should accept errors, since it would mean a bad config.

minrk · 2017-11-14T09:55:05Z

This is also implemented in #242, which applies to standard traitlets config instead of the JSONConfigManager used exclusively for extensions and frontend config.

I realize that we have a bit of complication here. The BaseJSONConfigManager as it is used for nbconfig is not at all related to traitlets, and probably doesn't belong in the traitlets package.

@blink1073 what do you think about whether this should be in traitlets, since it isn't related to traits at all? Should JupyterLab's extension loading live in jupyterlab instead?

maartenbreddels · 2017-11-14T15:14:55Z

Didn't see #242.
Yeah, I also wasn't sure this should be in traitlets, but it is also used in notebook.

blink1073 · 2017-11-14T19:38:43Z

I think it should go in the jupyter-server repo once we have one, but the notebook repo seems like a good place for now.

bollwyvl · 2017-12-07T18:20:35Z

💯 on getting this ready for some of the upcoming big releases.

I guess I would prefer the whole shooting match of #242, but since discussion is happening here...

On this PR, I like how all of the conf.d stuff is taken care of by the loader itself. Also how the conf.d's are read first, with the more user-serviceable named file coming last.

I guess it comes down to how the ordering should work.

Given this (admittedly weird) file layout:

$PREFIX/etc/jupyter/
  jupyter_notebook_config.py
  jupyter_notebook_config.json
  jupyter_notebook_config.d/
    a.json
    a.py
    b.json
    b.py
~/.jupyter/
  jupyter_notebook_config.py
  jupyter_notebook_config.json
  jupyter_notebook_config.d/
    a.json
    a.py
    b.json
    b.py

If each loader handled its own conf.d, it might be something like this:

$PREFIX/etc/jupyter/jupyter_notebook_config.d/a.py
$PREFIX/etc/jupyter/jupyter_notebook_config.d/b.py
$PREFIX/etc/jupyter/jupyter_notebook_config.py
$PREFIX/etc/jupyter/jupyter_notebook_config.d/a.json
$PREFIX/etc/jupyter/jupyter_notebook_config.d/b.json
$PREFIX/etc/jupyter/jupyter_notebook_config.json
~/.jupyter/jupyter_notebook_config.d/a.py
~/.jupyter/jupyter_notebook_config.d/b.py
~/.jupyter/jupyter_notebook_config.py
~/.jupyter/jupyter_notebook_config.d/a.json
~/.jupyter/jupyter_notebook_config.d/b.json
~/.jupyter/jupyter_notebook_config.json

vs the "big discovery" of #242 which i think would mean:

$PREFIX/etc/jupyter/jupyter_notebook_config.py
$PREFIX/etc/jupyter/jupyter_notebook_config.json
$PREFIX/etc/jupyter/jupyter_notebook_config.d/a.json
$PREFIX/etc/jupyter/jupyter_notebook_config.d/a.py
$PREFIX/etc/jupyter/jupyter_notebook_config.d/b.json
$PREFIX/etc/jupyter/jupyter_notebook_config.d/b.py
~/.jupyter/jupyter_notebook_config.py
~/.jupyter/jupyter_notebook_config.json
~/.jupyter/jupyter_notebook_config.d/a.json
~/.jupyter/jupyter_notebook_config.d/a.py
~/.jupyter/jupyter_notebook_config.d/a.json
~/.jupyter/jupyter_notebook_config.d/b.py

The py/json flipflop inside a configuration path due to file naming seems a bit counter to the Zen of Jupyter Config, but it's all pretty crazy at this point!

maartenbreddels · 2017-12-07T19:04:32Z

The failed travis is an issue with pytest and python 3.3. The other versions do build.

blink1073 · 2017-12-07T19:06:33Z

I'd say replace the py 3.3 test with a 3.6 one and update setup.py accordingly. @minrk, do you agree?

maartenbreddels · 2017-12-07T19:07:43Z

I think that makes sense, shall I do a separate PR for that?

blink1073 · 2017-12-07T19:08:10Z

Yeah that sounds good, thanks!

bollwyvl · 2017-12-07T19:15:14Z

traitlets/config/manager.py

+            # {section_name}.json take precedence.
+            # The idea behind this is that installing a Python package may
+            # put a json file somewhere in the a .d directory, while the 
+            # .json file is probably a user configuration.


maartenbreddels · 2017-12-07T19:19:21Z

If #462 gets merged I can trigger travis.

maartenbreddels · 2017-12-07T19:38:14Z

I've added some unittests assuming py.test, I hope that's fine.

blink1073 · 2017-12-07T20:15:27Z

Yes, py.test is the consensus for Jupyter projects.

maartenbreddels · 2017-12-07T20:20:43Z

Great. I'm really looking forward to seeing this (or equivalent functionality) being release. It would mean it is possible that a pip install ipywidget ipyvolume bqplot would make all these packages work directly in the jupyter notebook and jupyter lab (lab already works btw).

blink1073 · 2017-12-07T20:23:14Z

The serverextension part of lab does not yet work. We have a hack in the conda-forge package that uses post-link, but that is fragile and not supported by wheels and the default Anaconda packages.

maartenbreddels · 2017-12-07T20:35:49Z

Ah, indeed, most widget use the post-link as well, but that prevents noarch. So you can also use this PR for enabling the serverextension?

blink1073 · 2017-12-07T20:42:24Z

I believe so, using ~/.jupyter/jupyter_notebook_config.d/jupyterlab.json as:

{
  "NotebookApp": {
    "nbserver_extensions": {
      "jupyterlab": true
    }
}

Are we not merging down to that level?

maartenbreddels · 2017-12-07T21:26:21Z

Yes, I can confirm that works.

blink1073 · 2017-12-07T21:28:38Z

Awesome!

blink1073 · 2017-12-08T09:20:34Z

Actually, we had said earlier in the thread that the class should probably move to jupyter/notebook in its entirety. Does anyone object do doing that?

maartenbreddels · 2017-12-08T09:24:20Z

I don't. How do you want to not break old code, import the BaseJSONConfigManager from the manager.py file with a deprecation warning? Or just leave it untouched here?

maartenbreddels · 2017-12-08T09:36:32Z

Or, we get this merged, then consider moving it. Otherwise I'm afraid this functionality wil never get out.

blink1073 · 2017-12-08T10:33:10Z

I think we should be able to get a Notebook 5.3 out with this in it by the end of this month. Looking at the open issues for 5.0 in this repo I am less confident in a release.
I'd say we deprecate the class in this repo.

@minrk, thoughts?

…tlets#452

minrk · 2017-12-08T15:05:55Z

traitlets 5.0 is not very close to release, and without a lot of help, it's not going to be soon. There are way too many much higher priority things to deal with right now. I would encourage you as much as possible to take non-traitlets-specific config (e.g. jupyterlab extensions) entirely out of the traitlets repo.

maartenbreddels · 2017-12-08T15:14:54Z

Ok, we can close this then (continued here jupyter/notebook#3116)

blink1073 · 2017-12-08T15:38:05Z

@minrk, none of this is specific to JupyterLab. In fact, JupyterLab extensions use an entirely different mechanism. This is to make it easier for notebook and server extensions.

blink1073 · 2017-12-08T15:47:06Z

The reason JupyterLab came up in this conversation is that it uses a server extension.

allow default json files in a .d directory

06a3a38

blink1073 approved these changes Sep 1, 2017

View reviewed changes

minrk reviewed Nov 14, 2017

View reviewed changes

bollwyvl mentioned this pull request Dec 7, 2017

[WIP] Add conf.d config-loading #242

Closed

maartenbreddels added 2 commits December 7, 2017 19:50

cleanups/simplification after suggestions by @bollwyvl

9cae1ba

some comments on the order

5cc1365

bollwyvl reviewed Dec 7, 2017

View reviewed changes

test: add unittests for the BaseJSONConfigManager

1a37302

maartenbreddels added a commit to maartenbreddels/notebook that referenced this pull request Dec 8, 2017

allow default json files in a .d directory, original PR: ipython/trai…

89b32e4

…tlets#452

maartenbreddels mentioned this pull request Dec 8, 2017

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

Merged

maartenbreddels closed this Dec 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow default json files in a .d directory #452

allow default json files in a .d directory #452

maartenbreddels commented Sep 1, 2017 •

edited

Loading

minrk Nov 14, 2017

maartenbreddels Nov 14, 2017

bollwyvl Dec 7, 2017

maartenbreddels Dec 7, 2017

minrk commented Nov 14, 2017

maartenbreddels commented Nov 14, 2017

blink1073 commented Nov 14, 2017

bollwyvl commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

bollwyvl Dec 7, 2017

maartenbreddels commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

blink1073 commented Dec 8, 2017

maartenbreddels commented Dec 8, 2017

maartenbreddels commented Dec 8, 2017

blink1073 commented Dec 8, 2017

minrk commented Dec 8, 2017

maartenbreddels commented Dec 8, 2017

blink1073 commented Dec 8, 2017

blink1073 commented Dec 8, 2017

allow default json files in a .d directory #452

allow default json files in a .d directory #452

Conversation

maartenbreddels commented Sep 1, 2017 • edited Loading

minrk Nov 14, 2017

Choose a reason for hiding this comment

maartenbreddels Nov 14, 2017

Choose a reason for hiding this comment

bollwyvl Dec 7, 2017

Choose a reason for hiding this comment

maartenbreddels Dec 7, 2017

Choose a reason for hiding this comment

minrk commented Nov 14, 2017

maartenbreddels commented Nov 14, 2017

blink1073 commented Nov 14, 2017

bollwyvl commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

bollwyvl Dec 7, 2017

Choose a reason for hiding this comment

maartenbreddels commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

maartenbreddels commented Dec 7, 2017

blink1073 commented Dec 7, 2017

blink1073 commented Dec 8, 2017

maartenbreddels commented Dec 8, 2017

maartenbreddels commented Dec 8, 2017

blink1073 commented Dec 8, 2017

minrk commented Dec 8, 2017

maartenbreddels commented Dec 8, 2017

blink1073 commented Dec 8, 2017

blink1073 commented Dec 8, 2017

maartenbreddels commented Sep 1, 2017 •

edited

Loading