Unify behavior of server/nb extensions enabling, user vs sys.prefix #1508

Closed
bollwyvl opened this Issue Jun 2, 2016 · 23 comments

Comments

Projects
None yet
6 participants
@bollwyvl
Contributor

bollwyvl commented Jun 2, 2016

Users are encountering exciting conditions with the extension enabling behavior.

We encountered that here where we have basically had to say rm -rf ~/.jupyter.

The behavior of both the nbextensions and the serverextensions are surprising.

nbextensions: user and sys-prefix merge, but sys-prefix wins

$ jupyter nbextension enable thisthing --py --sys-prefix
$ jupyter nbextension enable thisotherthing --py --sys-prefix
$ jupyter nbextension disable thisthing --py
$ curl /api/config/notebook
{"this-thing/main": true, "this-other-thing/main": true}
$ jupyter nbextension enable this-thing --py
$ jupyter nbextension disable this-thing --py --sys-prefix
$ curl /api/config/notebook
{}

Good

  • merging works (of enabling)

Bad

  • user loses

potential fixes

# traitlets/config/manager.py
def recursive_update(target, new):
   ...
   if not target[k] is None: # was not updating with 'false' before
   ...

# notebook/services/config/manager.py
class ConfigManager(LoggingConfigurable):
    def get(self, section_name):
        for p in self.read_config_path[::-1]: # previously reading in reverse order

serverextensions: ~/.jupyter conquers all

$ jupyter serverextension enable this-thing --py --sys-prefix
$ jupyter serverextension enable this-other-thing --py --sys-prefix
$ jupyter serverextension disable this-thing --py
$ jupyter notebook
Collisions detected in jupyter_notebook_config.py and jupyter_notebook_config.json config files. jupyter_notebook_config.json has higher priority: {
      "NotebookApp": {
        "nbserver_extensions": "{'this-thing': True, 'this-other-thing': True} ignored, using {'this-thing': False}"
      }
    }

Good

  • user wins!

Bad

  • no merge

potential fixes

# notebook/services/config/manager.py
def _ensure_subconfig(self):
   ...
   if isinstance(obj, dict) \ # _is_section_key(key)
                    and not isinstance(obj, Config):

@minrk minrk added this to the 4.3 milestone Jun 3, 2016

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jun 8, 2016

Contributor

Do you propose that both follow:

  • Merge
  • User wins
Contributor

ellisonbg commented Jun 8, 2016

Do you propose that both follow:

  • Merge
  • User wins
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 8, 2016

Member

Yup, I think that makes good sense as a goal. Where we have some trouble is that the traitlets config system doesn't perform any merging of the values of individual traits, it only merges at the configurable-object level (i.e. it merges user_config.Config with sys_config.Config, but doesn't merge the contents of user_config.Config.value with sys_config.Config.value). While this isn't desirable for extension loading, it is most of the time. In some ways, this points to the traitlets config system simply being a bad fit for extensions, as we rapidly expand the scope of what configuration we are talking about enabling.

@SylvainCorlay discussed at the meeting a conf.d trait-type that would implement the directory-style loading without requiring changes to how traitlets.config works.

Alternately, we could move the serverextensions piece to use the exact same code as nbextensions, which would ensure the behavior of the two is totally consistent, rather than using traitlets.config for one and frontend-config for the other. Right now, I'm leaning a bit toward this option.

I think the nbextensions case is just a bug where the load order is backwards. That should be readily fixable.

Member

minrk commented Jun 8, 2016

Yup, I think that makes good sense as a goal. Where we have some trouble is that the traitlets config system doesn't perform any merging of the values of individual traits, it only merges at the configurable-object level (i.e. it merges user_config.Config with sys_config.Config, but doesn't merge the contents of user_config.Config.value with sys_config.Config.value). While this isn't desirable for extension loading, it is most of the time. In some ways, this points to the traitlets config system simply being a bad fit for extensions, as we rapidly expand the scope of what configuration we are talking about enabling.

@SylvainCorlay discussed at the meeting a conf.d trait-type that would implement the directory-style loading without requiring changes to how traitlets.config works.

Alternately, we could move the serverextensions piece to use the exact same code as nbextensions, which would ensure the behavior of the two is totally consistent, rather than using traitlets.config for one and frontend-config for the other. Right now, I'm leaning a bit toward this option.

I think the nbextensions case is just a bug where the load order is backwards. That should be readily fixable.

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jun 8, 2016

Contributor

@minrk but if both used the frontend extensions stuff, wouldn't that be an
API change as the actual location of the config would be different?

On Wed, Jun 8, 2016 at 1:59 AM, Min RK notifications@github.com wrote:

Yup, I think that makes good sense as a goal. Where we have some trouble
is that the traitlets config system doesn't perform any merging of the
values of individual traits, it only merges at the configurable-object
level (i.e. it merges user_config.Config with sys_config.Config, but
doesn't merge the contents of user_config.Config.value with
sys_config.Config.value). While this isn't desirable for extension loading,
it is most of the time. In some ways, this points to the traitlets config
system simply being a bad fit for extensions, as we rapidly expand the
scope of what configuration we are talking about enabling.

@SylvainCorlay https://github.com/SylvainCorlay discussed at the
meeting a conf.d trait-type that would implement the directory-style
loading without requiring changes to how traitlets.config works.

Alternately, we could move the serverextensions piece to use the exact
same code as nbextensions, which would ensure the behavior of the two is
totally consistent, rather than using traitlets.config for one and
frontend-config for the other. Right now, I'm leaning a bit toward this
option.

I think the nbextensions case is just a bug where the load order is
backwards. That should be readily fixable.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABr0Fj8CnIDObeEFoSSuJ4_w6ZL5tv_ks5qJoSBgaJpZM4Is9PS
.

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Contributor

ellisonbg commented Jun 8, 2016

@minrk but if both used the frontend extensions stuff, wouldn't that be an
API change as the actual location of the config would be different?

On Wed, Jun 8, 2016 at 1:59 AM, Min RK notifications@github.com wrote:

Yup, I think that makes good sense as a goal. Where we have some trouble
is that the traitlets config system doesn't perform any merging of the
values of individual traits, it only merges at the configurable-object
level (i.e. it merges user_config.Config with sys_config.Config, but
doesn't merge the contents of user_config.Config.value with
sys_config.Config.value). While this isn't desirable for extension loading,
it is most of the time. In some ways, this points to the traitlets config
system simply being a bad fit for extensions, as we rapidly expand the
scope of what configuration we are talking about enabling.

@SylvainCorlay https://github.com/SylvainCorlay discussed at the
meeting a conf.d trait-type that would implement the directory-style
loading without requiring changes to how traitlets.config works.

Alternately, we could move the serverextensions piece to use the exact
same code as nbextensions, which would ensure the behavior of the two is
totally consistent, rather than using traitlets.config for one and
frontend-config for the other. Right now, I'm leaning a bit toward this
option.

I think the nbextensions case is just a bug where the load order is
backwards. That should be readily fixable.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABr0Fj8CnIDObeEFoSSuJ4_w6ZL5tv_ks5qJoSBgaJpZM4Is9PS
.

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 9, 2016

Member

@ellisonbg yes, though it could be shimmed (the CLI API doesn't need to change). Any fix involving changes to existing traitlets behavior would be a more significant backward-incompatible change.

Member

minrk commented Jun 9, 2016

@ellisonbg yes, though it could be shimmed (the CLI API doesn't need to change). Any fix involving changes to existing traitlets behavior would be a more significant backward-incompatible change.

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Jun 9, 2016

Contributor

Perhaps having:

def merge(self, target, deep=False)

would allow this to work without breaking behavior.

If the conf.d implementation (still working on a poc) did live in
traitlets, used this approach (merge on the d files first, then the real
file) and had no knowledge of extensions per se, then an extension could
change anything, allowing things like nb_conda_ kernels (which I think
changes the kernel_spec_manager_class) to not need bespoke install steps.

Then the distinction between types of extensions would fall away... though
the existing cli method would still be good for user-level tweaking.

On 04:17, Thu, Jun 9, 2016 Min RK notifications@github.com wrote:

@ellisonbg https://github.com/ellisonbg yes, though it could be shimmed
(the CLI API doesn't need to change). Any fix involving changes to existing
traitlets behavior would be a more significant backward-incompatible change.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACxRBcOID_fZPEV_HHA6LeJ6om9mld_ks5qJ9ougaJpZM4Is9PS
.

Contributor

bollwyvl commented Jun 9, 2016

Perhaps having:

def merge(self, target, deep=False)

would allow this to work without breaking behavior.

If the conf.d implementation (still working on a poc) did live in
traitlets, used this approach (merge on the d files first, then the real
file) and had no knowledge of extensions per se, then an extension could
change anything, allowing things like nb_conda_ kernels (which I think
changes the kernel_spec_manager_class) to not need bespoke install steps.

Then the distinction between types of extensions would fall away... though
the existing cli method would still be good for user-level tweaking.

On 04:17, Thu, Jun 9, 2016 Min RK notifications@github.com wrote:

@ellisonbg https://github.com/ellisonbg yes, though it could be shimmed
(the CLI API doesn't need to change). Any fix involving changes to existing
traitlets behavior would be a more significant backward-incompatible change.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACxRBcOID_fZPEV_HHA6LeJ6om9mld_ks5qJ9ougaJpZM4Is9PS
.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 9, 2016

Member

A couple of questions for that proposal:

  • What would merge(deep=True) do with a list?
  • How would you recover the current behavior of strictly setting values of container traits? For example, c.Class.dict_trait = {}, which currently ensures the trait is an empty dict, overriding less-specific configuration.

We do have some of this functionality, just not in json config. Python config files do support modifications for container traits, which json config files do not. You can do:

c.Class.dict_trait.update({'a': 5})

and it will add values, rather than overriding the whole trait. We could try to define a scheme for expressing such operations in JSON config, though I am wary of defining operations in such a format, rather than values.

Re: nb_conda_kernels, I actually really don't want installing a package like that to be able to change the kernel manager. I really do think installing a package and modifying configuration should be separate steps in the vast majority of cases. What happens when I've installed two custom kernel managers, and they have registered themselves at install time, and assume that conda install is everything you need to do? With conf.d-style, I'm guessing the alphabetically last manager would be used. With post-link, it would at least be the most recently installed, probably. Neither one sounds like a good experience.

Member

minrk commented Jun 9, 2016

A couple of questions for that proposal:

  • What would merge(deep=True) do with a list?
  • How would you recover the current behavior of strictly setting values of container traits? For example, c.Class.dict_trait = {}, which currently ensures the trait is an empty dict, overriding less-specific configuration.

We do have some of this functionality, just not in json config. Python config files do support modifications for container traits, which json config files do not. You can do:

c.Class.dict_trait.update({'a': 5})

and it will add values, rather than overriding the whole trait. We could try to define a scheme for expressing such operations in JSON config, though I am wary of defining operations in such a format, rather than values.

Re: nb_conda_kernels, I actually really don't want installing a package like that to be able to change the kernel manager. I really do think installing a package and modifying configuration should be separate steps in the vast majority of cases. What happens when I've installed two custom kernel managers, and they have registered themselves at install time, and assume that conda install is everything you need to do? With conf.d-style, I'm guessing the alphabetically last manager would be used. With post-link, it would at least be the most recently installed, probably. Neither one sounds like a good experience.

@bollwyvl bollwyvl referenced this issue in conda-forge/conda-forge.github.io Jun 9, 2016

Closed

[discussion] should post-link/pre-unlink scripts output stderr/stdin? #173

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Jun 9, 2016

Member

@minrk I think I have a very simple proposal for a conf.d mechanism.

Imports:

from traitlets import List, Dict, default
from traitlets.config import LoggingConfigurable
from jupyter_core.paths import jupyter_config_path
import itertools
from glob import glob
import json
import os

A first simple version that does not take into account precedence:

class NotebookApp0(LoggingConfigurable):

    nbextensions = List(trait=Dict()).tag(config=True)

    @default('nbextensions')
    def _default_nbextensions(self):
        files = itertools.chain.from_iterable(glob(d + '/nbextensions.d/*.json') 
                                              for d in jupyter_config_path())
        return list(self.json_load(file) for file in files)

    def json_load(self, name):
        with open(name) as file:
            return json.load(file)

A version that takes into account directory precedence.

class NotebookApp1(LoggingConfigurable):

    nbextensions = List(trait=Dict()).tag(config=True)

    @default('nbextensions')
    def _default_nbextensions(self):
        dicts = reversed([{
            os.path.basename(v) : v for v in glob(d + '/nbextensions.d/*.json')
        } for d in jupyter_config_path()])
        return list({
            k: self.json_load(v) 
        for d in dicts for k, v in d.items() }.values())

    def json_load(self, name):
        with open(name) as file:
            return json.load(file)

Now if I have a file bqplot.json in ./jupyter/nbextensions.d/, the code

n = NotebookApp1()
n.nbextensions

Returns [{'dest': 'bqplot', 'require': 'bqplot/extension', 'src': 'static'}].

(the content of bqplot.json is)

{
    "src": "static",
    "dest": "bqplot",
    "require": "bqplot/extension"
}
Member

SylvainCorlay commented Jun 9, 2016

@minrk I think I have a very simple proposal for a conf.d mechanism.

Imports:

from traitlets import List, Dict, default
from traitlets.config import LoggingConfigurable
from jupyter_core.paths import jupyter_config_path
import itertools
from glob import glob
import json
import os

A first simple version that does not take into account precedence:

class NotebookApp0(LoggingConfigurable):

    nbextensions = List(trait=Dict()).tag(config=True)

    @default('nbextensions')
    def _default_nbextensions(self):
        files = itertools.chain.from_iterable(glob(d + '/nbextensions.d/*.json') 
                                              for d in jupyter_config_path())
        return list(self.json_load(file) for file in files)

    def json_load(self, name):
        with open(name) as file:
            return json.load(file)

A version that takes into account directory precedence.

class NotebookApp1(LoggingConfigurable):

    nbextensions = List(trait=Dict()).tag(config=True)

    @default('nbextensions')
    def _default_nbextensions(self):
        dicts = reversed([{
            os.path.basename(v) : v for v in glob(d + '/nbextensions.d/*.json')
        } for d in jupyter_config_path()])
        return list({
            k: self.json_load(v) 
        for d in dicts for k, v in d.items() }.values())

    def json_load(self, name):
        with open(name) as file:
            return json.load(file)

Now if I have a file bqplot.json in ./jupyter/nbextensions.d/, the code

n = NotebookApp1()
n.nbextensions

Returns [{'dest': 'bqplot', 'require': 'bqplot/extension', 'src': 'static'}].

(the content of bqplot.json is)

{
    "src": "static",
    "dest": "bqplot",
    "require": "bqplot/extension"
}
@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Jun 9, 2016

Member

This is much simpler than trying to have the whole configuration be based on configuration directories. Besides, we can enforce a schema for nbextension declarations via the trait and traits attributes of the nbextension config attribute.

Member

SylvainCorlay commented Jun 9, 2016

This is much simpler than trying to have the whole configuration be based on configuration directories. Besides, we can enforce a schema for nbextension declarations via the trait and traits attributes of the nbextension config attribute.

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Jun 9, 2016

Member

I am sure @minrk will find a clever way to make that dict comprehension even shorter.

Member

SylvainCorlay commented Jun 9, 2016

I am sure @minrk will find a clever way to make that dict comprehension even shorter.

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jun 9, 2016

Contributor

My question more pertains to the fact that the frontend and traditional
traitlets config files live in different locations and have significantly
different purposes and symantics (the frontend config can be written by the
live app, but only in the user area). I think we should aim to keep the
command line APIs the same, but don't know how to solve these problems
without:

  1. breaking the location and/or content of existing config files.
  2. Creating an even uglier implementation or model.

To address Nick's point - our answer to this point is that installation and
activation should always be separate. I know that people (including
ourselves) have figured out how to break that and automatically enable
things like ipywidgets when they are installed. I think we will regret that
in the long run for exactly the reasons you are stating. We should make
enabling extensions easier though - preferably through a nice UI.

On Thu, Jun 9, 2016 at 7:11 AM, Min RK notifications@github.com wrote:

A couple of questions for that proposal:

  • What would merge(deep=True) do with a list?
  • How would you recover the current behavior of strictly setting
    values of container traits? For example, c.Class.dict_trait = {},
    which currently ensures the trait is an empty dict, overriding
    less-specific configuration.

We do have some of this functionality, just not in json config. Python
config files do support modifications for container traits, which json
config files do not. You can do:

c.Class.dict_trait.update({'a': 5})

and it will add values, rather than overriding the whole trait. We could
try to define a scheme for expressing such operations in JSON config,
though I am wary of defining operations in such a format, rather than
values.

Re: nb_conda_kernels, I actually really don't want installing a package
like that to be able to change the kernel manager. I really do think
installing a package and modifying configuration should be separate
steps in the vast majority of cases. What happens when I've installed two
custom kernel managers, and they have registered themselves at install
time, and assume that conda install is everything you need to do? With
conf.d-style, I'm guessing the alphabetically last manager would be used.
With post-link, it would at least be the most recently installed, probably.
Neither one sounds like a good experience.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABr0J1k7R6p3WuZw9jQuLjwyQMfm_Erks5qKB7_gaJpZM4Is9PS
.

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Contributor

ellisonbg commented Jun 9, 2016

My question more pertains to the fact that the frontend and traditional
traitlets config files live in different locations and have significantly
different purposes and symantics (the frontend config can be written by the
live app, but only in the user area). I think we should aim to keep the
command line APIs the same, but don't know how to solve these problems
without:

  1. breaking the location and/or content of existing config files.
  2. Creating an even uglier implementation or model.

To address Nick's point - our answer to this point is that installation and
activation should always be separate. I know that people (including
ourselves) have figured out how to break that and automatically enable
things like ipywidgets when they are installed. I think we will regret that
in the long run for exactly the reasons you are stating. We should make
enabling extensions easier though - preferably through a nice UI.

On Thu, Jun 9, 2016 at 7:11 AM, Min RK notifications@github.com wrote:

A couple of questions for that proposal:

  • What would merge(deep=True) do with a list?
  • How would you recover the current behavior of strictly setting
    values of container traits? For example, c.Class.dict_trait = {},
    which currently ensures the trait is an empty dict, overriding
    less-specific configuration.

We do have some of this functionality, just not in json config. Python
config files do support modifications for container traits, which json
config files do not. You can do:

c.Class.dict_trait.update({'a': 5})

and it will add values, rather than overriding the whole trait. We could
try to define a scheme for expressing such operations in JSON config,
though I am wary of defining operations in such a format, rather than
values.

Re: nb_conda_kernels, I actually really don't want installing a package
like that to be able to change the kernel manager. I really do think
installing a package and modifying configuration should be separate
steps in the vast majority of cases. What happens when I've installed two
custom kernel managers, and they have registered themselves at install
time, and assume that conda install is everything you need to do? With
conf.d-style, I'm guessing the alphabetically last manager would be used.
With post-link, it would at least be the most recently installed, probably.
Neither one sounds like a good experience.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABr0J1k7R6p3WuZw9jQuLjwyQMfm_Erks5qKB7_gaJpZM4Is9PS
.

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Jun 9, 2016

Contributor

our answer to this point is that installation and activation should always be separate.

Right, for developers, but we had discussed that package managers need to have the ability to do this sensibly. I guess my downstream needs are now really driven by what it takes to deploy a (or hundreds) of running notebook servers each with bespoke configuration, and every extra step needed is a step that can break.

though I am wary of defining operations in such a format, rather than values.

Turns out there is a standard for doing just such a thing, as we have discussed over on nbdime, namely JSON Patch, built on JSON pointer, etc. The python-based configuration files are are really a lot harder to reason about, not sure how they figure into all this.

Contributor

bollwyvl commented Jun 9, 2016

our answer to this point is that installation and activation should always be separate.

Right, for developers, but we had discussed that package managers need to have the ability to do this sensibly. I guess my downstream needs are now really driven by what it takes to deploy a (or hundreds) of running notebook servers each with bespoke configuration, and every extra step needed is a step that can break.

though I am wary of defining operations in such a format, rather than values.

Turns out there is a standard for doing just such a thing, as we have discussed over on nbdime, namely JSON Patch, built on JSON pointer, etc. The python-based configuration files are are really a lot harder to reason about, not sure how they figure into all this.

@SylvainCorlay

This comment has been minimized.

Show comment
Hide comment
@SylvainCorlay

SylvainCorlay Jun 9, 2016

Member

What do you guys think of my implementation proposal above (which is simpler than what was proposed earlier IMO).

The registration of an nbextension would correspond to adding the corresponding file. Enabling would correspond to set an enabled attribute to true in its json file.

Member

SylvainCorlay commented Jun 9, 2016

What do you guys think of my implementation proposal above (which is simpler than what was proposed earlier IMO).

The registration of an nbextension would correspond to adding the corresponding file. Enabling would correspond to set an enabled attribute to true in its json file.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 10, 2016

Member

I think @SylvainCorlay's solution is a good one if we want to go the route of supporting this principle for extensions specifically, rather than for configuration as a whole.

If we want to do this more generally for configuration that is not this list-of-extensions structure in particular, then I'm guessing we probably shouldn't do both.

@ellisonbg I think we can take any of these approaches in a way that's compatible with 4.2, by updating the new-in-4.2 APIs to load into this structure. I don't think anyone is proposing the removal of the currently defined places to look for config, but a strict addition.

Since we already look for config in:

JUPYTER_CONFIG_PATH/config_name.{py,json}

The simplest implementation of conf.d in my mind is to add

JUPYTER_CONFIG_PATH/config_name.{py,json,d}

where all .py/.json (or we could limit it to .json) in JUPYTER_CONFIG_PATH/config_name.d/ are loaded in lexicographical order, as is the tradition of conf.d.

That would be quite a small change to traitlets.config. I implemented a simple version of this in ipython/traitlets#242. The trickier bit is the merge behavior that's actually the topic of this issue. I fixed the nbconfig side in #1526, since that was just a bug in iteration direction. The traitlets side would require doing something like @bollwyvl suggested with merge(deep=True). While I brought up some issues with this being a backward-incompatible change, that doesn't mean we shouldn't do it, and the affected cases are both rare and small. I also mentioned that merging is ill-defined for lists, so I would propose that we only support merging on dicts, which is easy enough.

Member

minrk commented Jun 10, 2016

I think @SylvainCorlay's solution is a good one if we want to go the route of supporting this principle for extensions specifically, rather than for configuration as a whole.

If we want to do this more generally for configuration that is not this list-of-extensions structure in particular, then I'm guessing we probably shouldn't do both.

@ellisonbg I think we can take any of these approaches in a way that's compatible with 4.2, by updating the new-in-4.2 APIs to load into this structure. I don't think anyone is proposing the removal of the currently defined places to look for config, but a strict addition.

Since we already look for config in:

JUPYTER_CONFIG_PATH/config_name.{py,json}

The simplest implementation of conf.d in my mind is to add

JUPYTER_CONFIG_PATH/config_name.{py,json,d}

where all .py/.json (or we could limit it to .json) in JUPYTER_CONFIG_PATH/config_name.d/ are loaded in lexicographical order, as is the tradition of conf.d.

That would be quite a small change to traitlets.config. I implemented a simple version of this in ipython/traitlets#242. The trickier bit is the merge behavior that's actually the topic of this issue. I fixed the nbconfig side in #1526, since that was just a bug in iteration direction. The traitlets side would require doing something like @bollwyvl suggested with merge(deep=True). While I brought up some issues with this being a backward-incompatible change, that doesn't mean we shouldn't do it, and the affected cases are both rare and small. I also mentioned that merging is ill-defined for lists, so I would propose that we only support merging on dicts, which is easy enough.

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Jun 10, 2016

Contributor

Thanks @minrk, @sylvancorley, looks like good stuff.

I am still in/am the camp of "let's configure it all," but appreciate
anything that moves us incrementally forward.

I think the list merging behavior is a fine thing to take off the table, as
not even a patch-like spec can solve that one satisfactorily. i think that
was part of the complexity we ran into with nbviewer entry_points pr.

@ellisonbg the ring 0 configurator/relauncher UI direction sounds good as
well! Sounds like the electron thing...

On 04:43, Fri, Jun 10, 2016 Min RK notifications@github.com wrote:

I think @SylvainCorlay https://github.com/SylvainCorlay's solution is a
good one if we want to go the route of supporting this principle for
extensions specifically, rather than for configuration as a whole.

If we want to do this more generally for configuration that is not this
list-of-extensions structure in particular, then I'm guessing we probably
shouldn't do both.

@ellisonbg https://github.com/ellisonbg I think we can take any of
these approaches in a way that's compatible with 4.2, by updating the
new-in-4.2 APIs to load into this structure. I don't think anyone is
proposing the removal of the currently defined places to look for config,
but a strict addition.

Since we already look for config in:

JUPYTER_CONFIG_PATH/config_name.{py,json}

The simplest implementation of conf.d in my mind is to add

JUPYTER_CONFIG_PATH/config_name.{py,json,d}

where all .py/.json (or we could limit it to .json) in
JUPYTER_CONFIG_PATH/config_name.d/ are loaded in lexicographical order, as
is the tradition of conf.d.

That would be quite a small change to traitlets.config. I implemented a
simple version of this in ipython/traitlets#242
ipython/traitlets#242. The trickier bit is the
merge behavior that's actually the topic of this issue. I fixed the
nbconfig side in #1526 #1526,
since that was just a bug in iteration direction. The traitlets side would
require doing something like @bollwyvl https://github.com/bollwyvl
suggested with merge(deep=True). While I b rought up some issues with
this being a backward-incompatible change, that doesn't mean we shouldn't
do it, and the affected cases are both rare and small. I also mentioned
that merging is ill-defined for lists, so I would propose that we only
support merging on dicts, which is easy enough.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACxRNadYPaKPXMxcPM7G89V9VhW-rLRks5qKTHWgaJpZM4Is9PS
.

Contributor

bollwyvl commented Jun 10, 2016

Thanks @minrk, @sylvancorley, looks like good stuff.

I am still in/am the camp of "let's configure it all," but appreciate
anything that moves us incrementally forward.

I think the list merging behavior is a fine thing to take off the table, as
not even a patch-like spec can solve that one satisfactorily. i think that
was part of the complexity we ran into with nbviewer entry_points pr.

@ellisonbg the ring 0 configurator/relauncher UI direction sounds good as
well! Sounds like the electron thing...

On 04:43, Fri, Jun 10, 2016 Min RK notifications@github.com wrote:

I think @SylvainCorlay https://github.com/SylvainCorlay's solution is a
good one if we want to go the route of supporting this principle for
extensions specifically, rather than for configuration as a whole.

If we want to do this more generally for configuration that is not this
list-of-extensions structure in particular, then I'm guessing we probably
shouldn't do both.

@ellisonbg https://github.com/ellisonbg I think we can take any of
these approaches in a way that's compatible with 4.2, by updating the
new-in-4.2 APIs to load into this structure. I don't think anyone is
proposing the removal of the currently defined places to look for config,
but a strict addition.

Since we already look for config in:

JUPYTER_CONFIG_PATH/config_name.{py,json}

The simplest implementation of conf.d in my mind is to add

JUPYTER_CONFIG_PATH/config_name.{py,json,d}

where all .py/.json (or we could limit it to .json) in
JUPYTER_CONFIG_PATH/config_name.d/ are loaded in lexicographical order, as
is the tradition of conf.d.

That would be quite a small change to traitlets.config. I implemented a
simple version of this in ipython/traitlets#242
ipython/traitlets#242. The trickier bit is the
merge behavior that's actually the topic of this issue. I fixed the
nbconfig side in #1526 #1526,
since that was just a bug in iteration direction. The traitlets side would
require doing something like @bollwyvl https://github.com/bollwyvl
suggested with merge(deep=True). While I b rought up some issues with
this being a backward-incompatible change, that doesn't mean we shouldn't
do it, and the affected cases are both rare and small. I also mentioned
that merging is ill-defined for lists, so I would propose that we only
support merging on dicts, which is easy enough.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1508 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACxRNadYPaKPXMxcPM7G89V9VhW-rLRks5qKTHWgaJpZM4Is9PS
.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 10, 2016

Member

Yeah, and any list-like configurable that wants merge can always switch from a list to a dict of bools, like we did with server extensions.

Member

minrk commented Jun 10, 2016

Yeah, and any list-like configurable that wants merge can always switch from a list to a dict of bools, like we did with server extensions.

Carreau added a commit that referenced this issue Aug 1, 2016

Backport PR #1526: reverse nbconfig load order
list is in descending priority, so load should iterate back to front, to ensure user config wins

the easy part of #1508

Carreau added a commit that referenced this issue Aug 1, 2016

Backport PR #1526: reverse nbconfig load order
list is in descending priority, so load should iterate back to front, to ensure user config wins

the easy part of #1508

minrk added a commit to minrk/notebook that referenced this issue Aug 2, 2016

Backport PR #1526: reverse nbconfig load order
list is in descending priority, so load should iterate back to front, to ensure user config wins

the easy part of #1508

minrk added a commit to minrk/notebook that referenced this issue Aug 3, 2016

Backport PR #1526: reverse nbconfig load order
list is in descending priority, so load should iterate back to front, to ensure user config wins

the easy part of #1508

minrk added a commit to minrk/notebook that referenced this issue Aug 3, 2016

Backport PR #1526: reverse nbconfig load order
list is in descending priority, so load should iterate back to front, to ensure user config wins

the easy part of #1508
@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Aug 5, 2016

Contributor

As part of this, perhaps we add a --conf-d flag to the various config changers?

jupyter serverextension enable foo --py --sys-prefix --conf-d=foo

to create a $PREFIX/etc/jupyter/jupyter_notebook_config.json.d/foo.json?

Additionally, more generally, I'd love a high-level config subcommand that knew how to work with any Application:

jupyter config nbconvert --sys-prefix --conf-d=foo \
  --NbConvertApp.executor_class=foo.KernelEnvExecutor [... more config]

or take a JSON file, I suppose.

This would remove the need for extension maintainers/packagers having to maintain things like foo.enable, where we have to use nasty business like BaseJSONConfigLoader, etc.

Additionally, jupyter config <subcommand> could show a pretty listing of exactly what would be used if the app was loaded, which would be really helpful for both supporting config issues, and creating/exporting reproducible configurations.

Contributor

bollwyvl commented Aug 5, 2016

As part of this, perhaps we add a --conf-d flag to the various config changers?

jupyter serverextension enable foo --py --sys-prefix --conf-d=foo

to create a $PREFIX/etc/jupyter/jupyter_notebook_config.json.d/foo.json?

Additionally, more generally, I'd love a high-level config subcommand that knew how to work with any Application:

jupyter config nbconvert --sys-prefix --conf-d=foo \
  --NbConvertApp.executor_class=foo.KernelEnvExecutor [... more config]

or take a JSON file, I suppose.

This would remove the need for extension maintainers/packagers having to maintain things like foo.enable, where we have to use nasty business like BaseJSONConfigLoader, etc.

Additionally, jupyter config <subcommand> could show a pretty listing of exactly what would be used if the app was loaded, which would be really helpful for both supporting config issues, and creating/exporting reproducible configurations.

@bollwyvl bollwyvl referenced this issue in Anaconda-Platform/nbpresent Sep 6, 2016

Open

Updating and uninstalling not working #84

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Nov 12, 2016

Member

Can we bump this from 4.3 to a later release? I'm aware that there are various fixes it would be good to get out to users, and this sounds like a bit of a bigger task.

Member

takluyver commented Nov 12, 2016

Can we bump this from 4.3 to a later release? I'm aware that there are various fixes it would be good to get out to users, and this sounds like a bit of a bigger task.

@minrk minrk modified the milestones: 4.4, 4.3 Nov 22, 2016

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Nov 22, 2016

Member

@takluyver bumped

Member

minrk commented Nov 22, 2016

@takluyver bumped

@ellisonbg ellisonbg modified the milestones: 5.0, 4.4 Dec 22, 2016

@minrk minrk modified the milestone: 5.0 Jan 13, 2017

@gnestor gnestor added this to the 5.0 milestone Feb 4, 2017

@gnestor gnestor removed this from Open Issues in 4.3 Feb 4, 2017

@gnestor gnestor removed this from Open issues in 4.4 Feb 4, 2017

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Feb 6, 2017

Member

@minrk Am I right that #2108 fixes/works around this as much as we intend to for 5.0?

Member

takluyver commented Feb 6, 2017

@minrk Am I right that #2108 fixes/works around this as much as we intend to for 5.0?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Feb 6, 2017

Member

@takluyver that's my understanding.

Member

minrk commented Feb 6, 2017

@takluyver that's my understanding.

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Feb 7, 2017

Contributor

@minrk can you summarize how all this will work as of 5.0? I want to make sure this stuff is sufficiently resolved that we don't have to change it again before moving to the conf.d approach.

Contributor

ellisonbg commented Feb 7, 2017

@minrk can you summarize how all this will work as of 5.0? I want to make sure this stuff is sufficiently resolved that we don't have to change it again before moving to the conf.d approach.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Feb 7, 2017

Member

@ellisonbg the only change for 5.0 is that nbserver_extensions from various config levels is merged in the same way as nbextensions, rather than clobbering at each level. Everything else is unchanged.

Member

minrk commented Feb 7, 2017

@ellisonbg the only change for 5.0 is that nbserver_extensions from various config levels is merged in the same way as nbextensions, rather than clobbering at each level. Everything else is unchanged.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Feb 7, 2017

Member

Closing here, since the titular issue is fixed: user wins, merge happens, nbextensions and serverextensions should behave the same now.

Member

minrk commented Feb 7, 2017

Closing here, since the titular issue is fixed: user wins, merge happens, nbextensions and serverextensions should behave the same now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment