Fixes #5002

[krinsman]

Deleting line 1278 fixes #5002

The rest of the deletions are code which was supposed to be removed with notebook 5.0, and it seems like notebook has advanced beyond that version.

[takluyver]

I suspect removing that line also breaks using --notebook-dir without setting the contents manager root dir in config. Interacting config options are hard.

[takluyver]

I suspect removing that line also breaks using --notebook-dir without setting the contents manager root dir in config. Interacting config options are hard.

[krinsman]

@takluyver To clarify, what is the intended behavior? And how do these changes break that behavior?

To me everything seems to work with the changes, which suggests I don't understand what the intended behavior is, and thus what is broken.

By default FileContents.root_dir takes its value from notebook_dir: https://github.com/jupyter/notebook/blob/master/notebook/services/contents/filemanager.py#L70

Right now, if both are specified by the user, only one of those specifications is listened to, which is rather confusing and frustrating (it took me 1-2 weeks of digging through the source code to figure out what was going on).

Being able to specify both independently is important e.g. when running jupyterhub-singleuser with a value of c.Spawner.notebook_dir different from what the user's content manager root directory is. Ideally what we want is for users to be able to view inside of notebook all files on the system, thus setting notebook_dir equal to root, but at the same time have notebook only attempt to write files inside of their home directory, thus the setting for ContentsManager.root_dir. Right now however the latter setting is ignored.

If it is actually intended that NotebookApp.notebook_dir == ContentsManager.root_dir always, no ifs, buts, etc., then it seems like ContentsManager.root_dir shouldn't be a configurable.

[takluyver]

Yuck, this is the kind of thing that makes me hate traitlets: the relationship between NotebookApp.notebook_dir and FileContents.root_dir is written in two completely separate bits of code, one of them without even mentioning NotebookApp directly. 😞

I suspect that the fact that setting them to different values gives you read-only access to files in one but not the other is a coincidence of how the implementation works, not something that's designed or guaranteed for the future.

[krinsman]

@takluyver So would it make sense to change this PR to instead make FileContents.root_dir non-configurable, since the intended behavior seems to be to always have NotebookApp.notebook_dir == ContentsManager.root_dir?

[krinsman]

@takluyver I updated the PR to just make the contents manager and kernel manager root directories non-configurable, since those settings are already silently overwritten by NotebookApp.notebook_dir anyway.

That being said it would probably be better if a deprecation warning for their non-configurability were inserted somehow -- I don't know what the best way would be to do that.

NotebookApp.notebook_dir, under the new name ServerApp.root_dir, as the "one root directory to rule them all" seems to be the direction the Jupyter Server project is already going anyway, at least to the best of my crude outsider understanding: jupyter-server/jupyter_server#36

[takluyver]

That would be my preferred direction - fewer config options that can interfere with one another. But as it's possible to set them separately at the moment, there's a good chance that someone has a use case for that, so it would be tricky to switch config off for those attributes now. :-(

[krinsman]

OK, so what if this PR was changed to create deprecation warnings saying that configuration for these traits is deprecated and will be removed in Notebook 7 or Notebook 8? As well as changes/additions to the Docs and docstrings saying that these will be deprecated? Would that be better?

Again, these traits are already de facto un-configurable, this is the gist of #5002, so it seems like it would be impossible for anyone to have a use case that depends on configuring them.

My main interest is in fixing #5002, because I lost a lot of time puzzling over that one before figuring out what the issue was, and it was an incredibly frustrating experience which I would prefer to ensure that future users can avoid. I really like Jupyter a lot and prefer that other people avoid frustrating experiences with it so that they can like Jupyter a lot too.

[takluyver]

Well, they are configurable when you don't use the --notebook-dir option.

I suspect that your original fix was probably the best idea - let the contents manager get the application option as part of calculating the default. It's just annoyingly hard to reason about these kind of interactions to verify that that will do the right thing in all situations - e.g. that it couldn't initialise the default before the config option is set on the application.

[krinsman]

OK @takluyver that makes sense to me too and I updated the PR with that strategy in mind.

The kernel manager and contents manager already automatically check the notebook app's notebook_dir for their default values of root_dir, so it seemed to me that the only code that needed to be changed to implement this was the code for the notebook app.

[takluyver]

Thanks for your patience; let's try it like that and see if anyone complains.