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

Remove hard-coding of jupyter_config_dir config_dir at the notebookapp level #882

Merged
merged 1 commit into from
Dec 18, 2015

Conversation

damianavila
Copy link
Member

Let's the ConfigManager to deal with the default value, so we can make it truly configurable.

Pretend to fix #880

…p level, letting that being managed ny the ConfigManager.
@damianavila
Copy link
Member Author

Can we make this into 4.1? It is a bug fix from my point of view and more importantly, this allow to use custom ConfigManager, which provides an alternative until things related with the nbextensions discussion in #878 and jupyter/enhancement-proposals#7 is decided.
ping @minrk @ellisonbg @takluyver and @Carreau

@minrk
Copy link
Member

minrk commented Dec 18, 2015

It's a reasonable question. I'm not sure if we want to actually make that configurable, or remove the config=True flag that doesn't actually work, but we should do one or the other. Allowing sub-config locations to be themselves configurable makes it a lot easier for it to get into a compliated and inconsistent state. I would like to see a pretty good reason for frontend config and server config to have to be in different locations for these to be allowed to be separated.

@Carreau
Copy link
Member

Carreau commented Dec 18, 2015

I'm fine with this fix. Should we merge that and Beta2 or RC ? Min ?

@minrk
Copy link
Member

minrk commented Dec 18, 2015

I'm fine with it as well, I'd just like a bit more explanation of cases where it's reasonable for frontend config to come from a different place from the rest.

@Carreau
Copy link
Member

Carreau commented Dec 18, 2015

Like other place from the disk ? I'm sure Continuum have deployment where config is on a db, or a separate folder network-mounted, or sync via dropbox.

@minrk
Copy link
Member

minrk commented Dec 18, 2015

This is only relevant for breaking up user-config, so that part of user-config can come from JUPYTER_CONFIG_DIR, but the nbconfig subdir is broken out of the config dir.

Arbitrary other data sources already works fine by specifying NotebookApp.config_manager_class.

I'm mainly wondering what the case is where you would want to specify where nbconfig is, but not JUPYTER_CONFIG_DIR.

@damianavila
Copy link
Member Author

OK, I (and others) have written some nbextensions and server extensions and package them with conda. Because we want to keep thing isolated inside prefix, we want to read the config from prefix/etc/jupyter/nbconfig but some people have also some other extensions installed in the user space (aka. ./jupyter).
If we setup the jupyter_config_dir to just look into prefix/etc/jupyter all the "user" activated extensions are not loaded... this is the main discussion in the other issues.

To workaround this for the moment I have written a custom ConfigManager which is able to read the "user" space but also the prefix space, so people can install our extensions and also install other extensions as well and have all of them activated beyond the place where the the config is located.

In fact, I have no problem to avoid config=True at the native ConfigManager, we can remove that if you feel it will bring problems. The main thing is avoid hardcoding the config_dir at the notebookapp level so anyone can come up with another ConfigManager implementation to support their use case (like the one I described above) or any other use case that the native implementation does not support...

@minrk
Copy link
Member

minrk commented Dec 18, 2015

OK

minrk added a commit that referenced this pull request Dec 18, 2015
Remove hard-coding of jupyter_config_dir config_dir at the notebookapp level
@minrk minrk merged commit 4960c3f into jupyter:master Dec 18, 2015
@minrk minrk added this to the 4.1 milestone Dec 18, 2015
@damianavila damianavila deleted the fix_config_dir branch December 18, 2015 14:24
@ellisonbg
Copy link
Contributor

After working on this stuff yesterday, I have doubts about doing this ... Will reply more later when I fully wake up...

Sent from my iPhone

On Dec 18, 2015, at 6:20 AM, Min RK [email protected] wrote:

Merged #882.


Reply to this email directly or view it on GitHub.

@damianavila
Copy link
Member Author

Thanks!

I still believe we should continue discussing the whole thing in the other threads and the meeting, but at least this one gives people an entry point to workaround the whole thing if the resolution we come up later do not satisfy their requirements.

Carreau added a commit to Carreau/jupyter_notebook that referenced this pull request Dec 24, 2015
though, jupyter#893 argues that we might need it and that it will be complex to
reintroduce it.

This use a convoluted way to :

 - warn that this kwarg might not be given to config managers in future version
 - but do not warn on default installs with default config.
 - warn that the keyword is ignored when people use subclasses of out
   config manager, and pass the keyword to super().

Thus this actually advertise that the keyword **might** be removed,
by still allowing us to keep it if in the end it appears that we need
it.

Should help with jupyter#893 without un-fixing jupyter#882.
Carreau added a commit to Carreau/jupyter_notebook that referenced this pull request Dec 24, 2015
though, jupyter#893 argues that we might need it and that it will be complex to
reintroduce it.

This use a convoluted way to :

 - warn that this kwarg might not be given to config managers in future version
 - but do not warn on default installs with default config.
 - warn that the keyword is ignored when people use subclasses of out
   config manager, and pass the keyword to super().

Thus this actually advertise that the keyword **might** be removed,
by still allowing us to keep it if in the end it appears that we need
it.

Should help with jupyter#893 without un-fixing jupyter#882.
Carreau added a commit to Carreau/jupyter_notebook that referenced this pull request Dec 24, 2015
though, jupyter#893 argues that we might need it and that it will be complex to
reintroduce it.

This use a convoluted way to :

 - warn that this kwarg might not be given to config managers in future version
 - but do not warn on default installs with default config.
 - warn that the keyword is ignored when people use subclasses of out
   config manager, and pass the keyword to super().

Thus this actually advertise that the keyword **might** be removed,
by still allowing us to keep it if in the end it appears that we need
it.

Should help with jupyter#893 without un-fixing jupyter#882.
Carreau added a commit to Carreau/jupyter_notebook that referenced this pull request Dec 24, 2015
Pr jupyter#882 removed `config_dir` kwarg as it was ignored.

though, jupyter#893 argues that we might need it and that it will be complex to
reintroduce it.

This use a convoluted way to :

 - warn that this kwarg might not be given to config managers in future version
 - but do not warn on default installs with default config.
 - warn that the keyword is ignored when people use subclasses of out
   config manager, and pass the keyword to super().

Thus this actually advertise that the keyword **might** be removed,
by still allowing us to keep it if in the end it appears that we need
it.

Should help with jupyter#893 without un-fixing jupyter#882.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some problems with the ConfigManager
4 participants