Remove configurability of config_dir in nbconfig

[ellisonbg]

Hi all. It is extremely important to not release 4.1 with the recent change to the config_dir attribute of the nbconfig manager. That change makes the work I am doing for 4.2 on nbextensions backwards incompatible, so we couldn't release it until 5.0. We should just revert it for 4.1.

[damianavila]

The recent change did not change the configurable status of the config manager...
The config_dir was hardcoded at the notebookapp level and now it resides in the ConfigManager...
If you remove this line https://github.com/jupyter/notebook/blob/master/notebook/services/config/manager.py#L15, you make the ConfigManager not configurable but still you allow people to subclass the ConfigManager to suit it their need... that'e enough for you? I think so... I can submit a PR with that change...

[ellisonbg]

Can you look at my PR - the entire API of that class has changed in my PR.
Don't have time to look right now myself to understand this...

On Tue, Dec 22, 2015 at 11:26 AM, Damián Avila notifications@github.com
wrote:

The recent change did not change the configurable status of the config
manager...
The config_dir was hardcoded at the notebookapp level and now it resides
in the ConfigManager...
If you remove this line
https://github.com/jupyter/notebook/blob/master/notebook/services/config/manager.py#L15,
you make the ConfigManager not configurable but still you allow people to
subclass the ConfigManager to suit it their need... that'e enough for you?
I think so... I can submit a PR with that change...

—
Reply to this email directly or view it on GitHub
#893 (comment).

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

[damianavila]

Can you look at my PR - the entire API of that class has changed in my PR.

I will take a look in the next few hours...

[damianavila]

Here you are removing the config_dir configurable and removing the "private" method to conform a new API... so if we just remove the config_dir configurable from the ConfigManager, I mean removing just the line 15: https://github.com/jupyter/notebook/blob/master/notebook/services/config/manager.py#L15, then you will be backward compatible with 4.1 because you will be only removing the "private" method... So things keep in the same way from the outside viewer... NOT a --ConfigManager.config_dir option available to use and the native ConfigManager pointing to the user config_dir... or am I missing something else?

[ellisonbg]

IIRC, I changed the base class of the manager though. I am no longer using
the same class as the base class...also, when I do instantiate the base
class I don't pass it config. If you are relying on configuring the base
class, that will stop working in the future.

On Tue, Dec 22, 2015 at 12:39 PM, Damián Avila notifications@github.com
wrote:

Here https://github.com/jupyter/notebook/pull/879/files#r48294796 you
are removing the config_dir configurable and removing the "private"
method to conform a new API... so if we just remove the config_dir
configurable from the ConfigManager, I mean removing just the line 15:
https://github.com/jupyter/notebook/blob/master/notebook/services/config/manager.py#L15,
then you will be backward compatible with 4.1 because you will be only
removing the "private" method... So things keep in the same way from the
outside viewer... NOT a --ConfigManager.config_dir option available to
use and the native ConfigManager pointing to the user config_dir... or am I
missing something else?

—
Reply to this email directly or view it on GitHub
#893 (comment).

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

[Carreau]

For information you are referring to #882 I guess. (and #879)

[Carreau]

I might be missing something, but I tend to side with @damianavila on this one.
Worse case, you can do BaseJSONConfigManager(Config({"config_dir":p})), as anyway BaseJSONConfigManager cannot take a config_dir kwarg in traitlets 4.0, and we will likely not require traitlets 4.1 for notebook 4.2.

[Carreau]

@damianavila
Would passing config_dir=os.path.join(self.config_dir, 'nbconfig'), again, but ignoring it in the base config class suit both of you ?

[damianavila]

@Carreau, passing config_dir=os.path.join(self.config_dir, 'nbconfig') where? At the notebookapp level? That's not good... if we hardcode the config_dir again we can no longer use a custom ConfigManager...

Or are you proposing to remove _config_dir_default and make the config_dir=os.path.join(self.config_dir, 'nbconfig') there, inside the ConfigManager class?

$ git diff
diff --git a/notebook/services/config/manager.py b/notebook/services/config/manager.py
index 2399f86..7ccd623 100644
--- a/notebook/services/config/manager.py
+++ b/notebook/services/config/manager.py
@@ -7,11 +7,9 @@ import os.path

 from traitlets.config.manager import BaseJSONConfigManager
 from jupyter_core.paths import jupyter_config_dir
-from traitlets import Unicode

 class ConfigManager(BaseJSONConfigManager):
     """Config Manager used for storing notebook frontend config"""

-    config_dir = Unicode(config=True)
-    def _config_dir_default(self):
-        return os.path.join(jupyter_config_dir(), 'nbconfig')
+    config_dir = os.path.join(jupyter_config_dir(), 'nbconfig')

If that's the case, we can probably live with it 😉

[Carreau]

Then I think I'm confused of what Brian ask to revert, and what he is asking to re-add for 4.1.

I'm wondering what would be a good compromise for 4.1 that would allow Brian to do his work without being impaired and not risking API breakage.

[damianavila]

Then I think I'm confused of what Brian ask to revert, and what he is asking to re-add for 4.1.

I think I am in the same position, I don't get where is the issue... despite the fact that before my PR the ConfigManager.config_dir did not work and now it is working... so, to go back to a "similar" situation than before, but without passing the config_dir at the notebook level, we just need to make config_dir at the ConfigManager not a configurable and you can do that with the diff I posted above, I think...

I'm wondering what would be a good compromise for 4.1 that would allow Brian to do his work without being impaired and not risking API breakage.

Not sure if I am completely understanding Brian, but I guess the diff posted above is a good compromise IF I understood him well...

[ellisonbg]

Let me have a look...

Sent from my iPhone

On Dec 24, 2015, at 11:31 AM, Damián Avila notifications@github.com wrote:

Then I think I'm confused of what Brian ask to revert, and what he is asking to re-add for 4.1.

I think I am in the same position, I don't get where is the issue... despite the fact that before my PR the ConfigManager.config_dir did not work and now it is working... so, to go back to a "similar" situation than before, but without passing the config_dir at the notebook level, we just need to make config_dir at the ConfigManager not a configurable and you can do that with the diff I posted above, I think...

I'm wondering what would be a good compromise for 4.1 that would allow Brian to do his work without being impaired and not risking API breakage.

Not sure washout completely understanding Brian, but I guess the diff posted above is a good compromise IF I understood him well...

—
Reply to this email directly or view it on GitHub.

[ellisonbg]

OK, here is a summary of the past and current state:

In 4.0:

• NotebookApp.config_manager_class is a configurable that the notebook app uses to instantiate self.config_manager.
• This instances is used in the config handler (it gets passed to the tornado settings and ends up as an attribute on the handler) to set/get config from the frontend. But the config_dir attribute is not used in that API.
• In 4.0, we hard-code the config_dir attribute of the config manager when the NotebookApp instantiates it. Because of this, @damianavila can swap out the configurable config_manager_class, but can't set which config dir is used.

In 4.1 master:

• NotebookApp.config_manager_class is still used to instantiate the class that is used in the config handler.
• But we no longer hard-code the config_dir attribute at that location in the NotebookApp.
• Now the default comes from the ConfigManager class itself, which has a _config_dir_default method that has been there all along, but wasn't being used because we were passing it in.

My proposal for 4.2:

• I pull a sleight of hand to load config from multiple directories in a backwards compatible manner.
• My ConfigManager class has the exactly same API that is used by the handlers.
• But, it doesn't have a config_dir attribute at all because it ends up loading from multiple directories.

My conclusion:

• If we consider config_dir to be part of the public API in 4.0, then my entire approach falls apart (it is backwards incompatible).
• But because of the bug that was in 4.0, it was mostly impossible to pass in a custom config manager that uses a different config_dir.
• Because of that bug, I am not sure we should consider any of this as being public API in 4.0.
• But the bug fix of @damianavila does make it all public (in current master).
• If someone uses a custom ConfigManager that subclasses the one from 4.1, my 4.2 approach will likely break their code.

Summary: a bug in 4.0 essentially hides an API from users/devs in a way that allows me to change it without breaking backwards compatibility. The bug fix in 4.1 will make my 4.2 proposal backwards incompatible.

But, I don't think anyone is using any of this stuff other than @damianavila et al. So if he is fine moving forward with the current state of 4.1 and is fine with my proposal for 4.2 that will break his 4.1 stuff, then I am OK.

My main concern here is that I don't want us to be prevented from improving the nbextension situation for users in 4.2, just because of an obscure, unused API.

[minrk]

Would you prefer to revert the configurability fix, if I show a workaround that will work on 4.0?

import os

from traitlets import Unicode

from jupyter_core.paths import jupyter_config_dir
from notebook.services.config import ConfigManager

class MyConfigManager(ConfigManager):
    custom_config_dir = Unicode(config=True)
    def _custom_config_dir_default(self):
        return os.path.join(jupyter_config_dir(), 'nbconfig')

    @property
    def config_dir(self):
        # config_dir is a read-only alias to custom_config_dir
        return self.custom_config_dir

    @config_dir.setter
    def config_dir(self, value):
        # ignore attempts to set config_dir
        pass

c.MyConfigManager.custom_config_dir = '/tmp/custom'

c.NotebookApp.config_manager_class = MyConfigManager

This way a custom_config_dir configurable is added, which can set the config directory, and works around any attempt by the NotebookApp to set the default config_dir.

[minrk]

If that's the way we want to go, #905 restores the hardcoded value.

[ellisonbg]

Actually @damianavila I have a question. What are you trying to do in your custom ConfigManager subclass. That may be the most important thing to address here.

The reason I say that is this - if you are going to ship a custom ConfigManager that only reads config from the conda specific config location, and not the user's config, that would be very frustrating to users. It means that all nbextensions that users have installed and working in 4.0 will stop working in 4.1 if they are using conda. I think that would be a step backwards.

If, on the other hand, you are going to ship a custom ConfigManager that does what the one in my PR does (loads from all locations, but saves only in user config) then you won't break user's existing installs.

These issues are much more important to me than the particular details of the changes we are discussing making to 4.1.

[damianavila]

@minrk thanks for the workaround... in fact, I workaround the issue in another way just setting the config_dir at the initialization of the custom ConfigManger and it seems to work OK...

@ellisonbg, that's the whole point of the discussion, we are trying to install the extension inside prefix but also enabling the user installed extension... that's the reason for our custom ConfigManager which is essentially reading the config from the user and "adding" it to the config living in the prefix, so people starting with jupyter notebook in a conda environment where our extension are installed can also use their own extension ("activated" at the user config place)

If, on the other hand, you are going to ship a custom ConfigManager that does what the one in my PR does (loads from all locations, but saves only in user config) then you won't break user's existing installs.

As I mentioned before, the implementation is a little bit different because we copy the content of the config files at the user space and add those to the config inside the env... so, in this way, we are not modifying the user space from inside the env (because that would break the env isolation and play dirty with something that only the user should modify - and not a package manager)... finally, we point to the env's config dir to start the notebook...

[ellisonbg]

@damianavila I want to understand this. Are you actively migrating user config to the sys.prefix config in the conda env?

The problem with that is that the builtin jupyter CLI tools (jupyter nbextension) in 4.1 don't have any way to manipulate the config files in sys.prefix. Thus, a user would have no way to disable nbextensions that you have migrated without diving into obscure directories in the conda env.

Why not just use the ConfigManager in my PR that loads from all directories, but writes only to the sys.prefix one? Then you will already be using the official approach we will support in 4.2.

[damianavila]

@damianavila I want to understand this. Are you actively migrating user config to the sys.prefix config in the conda env?

Yes.

The problem with that is that the builtin jupyter CLI tools (jupyter nbextension) in 4.1 don't have any way to manipulate the config files in sys.prefix. Thus, a user would have no way to disable nbextensions that you have migrated without diving into obscure directories in the conda env.

We started to work on this several weeks/months ago and we developed some other tools to potentially solve this issue because the jupyter nbextension tools were not prepared to deal with environments... in the road we learn some things and this is why we pushed the discussion in some nbextension-related issues lately...

Why not just use the ConfigManager in my PR that loads from all directories, but writes only to the sys.prefix one?

Now, this is an option! Because we had several discussion and some consensus and your PR is now out there... From an historical perspective, so you can understand how this evolved, we put in place a machinery for some internal work when the discussion and consensus did not even birth yet... and we solved some issues on the road with custom developments.
With these things we can provide now some good experience around nbextension and server-extension for conda users...
But, we would like/should adopt the consensus and migrate to something more general in the near future... like the thing you proposed in your PR.

Then you will already be using the official approach we will support in 4.2.

That's interesting and it is in my todo list to do it if I am not redirected to other efforts inside the company 😉