Make preferred_dir content manager trait

[vidartf]

Move the preferred_dir trait to content manager, but keep the old one for backwards compatibility. The new location should also cause the value to be read later in the init, allowing us to avoid the deferred validation. Also fixes an issue with escaping the root dir on Windows if an absolute path is passed.

[vidartf]

@kevin-bates highlighted to check if a change needs to happen in JupyterLab to pick up the value from the new trait or not

[kevin-bates]

Thanks for submitting this PR @vidartf!

I had a chance to run with your branch a bit and agree these related (and duplicated) trait scenarios are difficult!

If I run server with a preferred_dir that exists, but as a sibling to root_dir, that config violation does not appear to be detected until Lab goes to fetch preferred_dir from the contents service. For example, this command line:

$ jupyter lab --ServerApp.preferred_dir=~/dev --ServerApp.root_dir=~/notebooks

will produce this output...

...
    To access the server, open this file in a browser:
        file:///Users/kbates/Library/Jupyter/runtime/jpserver-59158-open.html
    Or copy and paste one of these URLs:
        http://localhost:8888/lab?token=911cbde8ca5eba77311ecd2019f2022a67d6df6461766d71
     or http://127.0.0.1:8888/lab?token=911cbde8ca5eba77311ecd2019f2022a67d6df6461766d71
[W 2022-09-15 11:45:35.671 ServerApp] 404 GET /api/contents/dev?content=1&1663267535664 (::1): file or directory does not exist: 'dev'
[W 2022-09-15 11:45:35.672 ServerApp] wrote error: "file or directory does not exist: 'dev'"
[W 2022-09-15 11:45:35.672 ServerApp] 404 GET /api/contents/dev?content=1&1663267535664 (95b61f48f2ef4d63bb464bbf09ba3f32@::1) 1.50ms referer=http://localhost:8888/lab

with this dialog in Lab (which I know is unrelated but thought I'd include)...

However, setting preferred_dir on the CM (or FileContentsManager) via the CLI does catch things early...

$ jupyter lab --ContentsManager.preferred_dir=~/dev --ServerApp.root_dir=~/notebooks
...
[C 2022-09-15 11:50:30.021 ServerApp] Bad config encountered during initialization: No such directory: ''/Users/kbates/dev''

((I wonder if we should change this messaging to something like f"Preferred_dir {value} does not exist or is not a sub-directory or root_dir"?))

I'm also not seeing the deprecation warning under any circumstances - nor hitting a breakpoint in FileContentsManager._default_preferred_dir(). I do hit the validator in ServerApp when defining --ServerApp.preferred_dir and wonder if the contents manager's validator should just duplicate the one in ServerApp (and not defer to super()) and move the deprecation warning to that ServerApp._preferred_dir_validate()?

[codecov-commenter]

Codecov Report

Base: 80.01% // Head: 80.06% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (5fffaf7) compared to base (f52aa71).
Patch coverage: 94.87% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #983      +/-   ##
==========================================
+ Coverage   80.01%   80.06%   +0.04%     
==========================================
  Files          68       68              
  Lines        8021     8040      +19     
  Branches     1587     1588       +1     
==========================================
+ Hits         6418     6437      +19     
  Misses       1181     1181              
  Partials      422      422              

Impacted Files	Coverage Δ
jupyter_server/services/contents/filemanager.py	75.27% <91.30%> (+0.71%)	⬆️
jupyter_server/serverapp.py	80.00% <100.00%> (-0.08%)	⬇️
jupyter_server/services/contents/fileio.py	90.10% <100.00%> (+0.10%)	⬆️
jupyter_server/services/contents/manager.py	86.74% <100.00%> (+0.41%)	⬆️
jupyter_server/services/kernels/kernelmanager.py	81.11% <0.00%> (-0.31%)	⬇️
...ter_server/services/kernels/connection/channels.py	59.60% <0.00%> (-0.23%)	⬇️
jupyter_server/utils.py	85.71% <0.00%> (+0.59%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[vidartf]

@kevin-bates highlighted to check if a change needs to happen in JupyterLab to pick up the value from the new trait or not

Yes, a change to jupyterlab-server would be needed here: https://github.com/jupyterlab/jupyterlab_server/blob/4263852881fd2bd181712dcabac1bda176886cd5/jupyterlab_server/handlers.py#L90-L99

[kevin-bates]

Thanks for the update @vidartf.

Lately, I've been hearing that it's not uncommon for a given installation to be using multiple instances (and types) of ContentsManager simultaneously. For references like that above in jupyterlab_server, would only the ContentsManager that is configured on ServerApp be considered the ContentsManager from which preferred_dir is used?

[vidartf]

Lately, I've been hearing that it's not uncommon for a given installation to be using multiple instances (and types) of ContentsManager simultaneously. For references like that above in jupyterlab_server, would only the ContentsManager that is configured on ServerApp be considered the ContentsManager from which preferred_dir is used?

Lab server would read out the preferred dir from the main/root contents manager, and pass that path to JupyterLab. That path could ofc then be a path into any proxied "inner" contents manager. That would be an internal detail for them to sort out.

[vidartf]

If I run server with a preferred_dir that exists, but as a sibling to root_dir, that config violation does not appear to be detected until Lab goes to fetch preferred_dir from the contents service.

Given that these traits are being deprecated, I think it will probably be okay if their validation behavior is not the most user friendly, as long as it is eventually correct. But I will have a look to see if I can force a validation at some point during startup if the deprecated traits are being set.

(I wonder if we should change this messaging to something like f"Preferred_dir {value} does not exist or is not a sub-directory or root_dir"?)

Makes sense.

I'm also not seeing the deprecation warning under any circumstances - nor hitting a breakpoint in FileContentsManager._default_preferred_dir().

I'll have a look.

[vidartf]

Please test it with jupyterlab/jupyterlab_server#324

[kevin-bates]

Thanks @vidartf - I'll try to take this (and 324) for a spin this afternoon.

[kevin-bates]

Hi @vidartf - the changes look good. In one of the comments I provide analysis relative to an arguably questionable scenario that winds up in a rabbit hole - although I think the jupyterlab_server handler could be a bit more robust, I just wasn't sure if errors can be raised at that level.

[vidartf]

@kevin-bates Thanks for the review. I pushed a new commit here, and one on the jupyterlab_server branch to help address some of the concerns. I didn't end up changing the warning stack level after all, since I figured it was better to be consistent with other deprecations:

<env dir>\lib\site-packages\traitlets\traitlets.py:1752: DeprecationWarning: ServerApp.token config is deprecated in jupyter-server 2.0. Use IdentityProvider.token
  return self._get_trait_default_generator(names[0])(self)
[I 2022-10-15 14:58:48.075 ServerApp] notebook_shim | extension was successfully linked.
<env dir>\lib\site-packages\traitlets\traitlets.py:1752: DeprecationWarning: ServerApp.preferred_dir config is deprecated in jupyter-server 2.0. Use ContentsManager.preferred_dir with a relative path instead
  return self._get_trait_default_generator(names[0])(self)
[I 2022-10-15 14:58:48.196 ServerApp] notebook_shim | extension was successfully loaded.
[I 2022-10-15 14:58:48.199 ServerApp] jupyter_server_terminals | extension was successfully loaded.
<env dir>\lib\site-packages\traitlets\traitlets.py:1752: DeprecationWarning: ServerApp.token config is deprecated in jupyter-server 2.0. Use IdentityProvider.token
  return self._get_trait_default_generator(names[0])(self)

[kevin-bates]

Thanks for the updates. I'm finding the test failures getting tangled between catching SystemExit and TraitError. It seems the exceptions are occurring from within the jp_configurable_serverapp fixture so I found it needed to be moved within the with pytest.raises blocks.

Here's the commit. I was hesitant to push it in case you wanted to take a different approach or this behavior is indicative of another issue.

[vidartf]

@kevin-bates You commit looks good to me! Please push it onto the branch 👍

[kevin-bates]

@vidartf - My commit has been pushed. The merge was more involved than I expected. The tests passed locally, but I'm hoping I didn't mess anything up. 😊

[vidartf]

@kevin-bates I did a rebase anyway to pull in latest changes from main. I took the opportunity to clean up the commit order then.

[kevin-bates]

I pushed a change to satisfy pre-commit. What's odd is that the set of pre-commit actions used in CI is different than what is used in the git hook. The latter doesn't call flake8-pyproject or doc8. I haven't looked into why/where that difference occurs.

[blink1073]

The ones that are listed as hook-stage: manual are only run in CI by default, because they can't be fixed automatically, and we didn't want them to prevent pushing.

You can run pre-commit run --hook-stage manual locally

[vidartf]

The latest merge from master highlights that I was missing a sync/async compat for the dir_exists call, which is good. However, I do not know how to do this inside a trait validator, since AFAIK, traitlets do not support asynchronous validators.

[vidartf]

@blink1073 If you did mean the ioloop one, that raises:

jupyter_server\services\contents\manager.py:92: in _validate_preferred_dir
    dir_exists = self.parent.io_loop.run_sync(self.dir_exists(value))
C:\cleanconda\envs\server-dev\lib\site-packages\tornado\ioloop.py:524: in run_sync
    self.start()
C:\cleanconda\envs\server-dev\lib\site-packages\tornado\platform\asyncio.py:199: in start
    self.asyncio_loop.run_forever()
C:\cleanconda\envs\server-dev\lib\asyncio\base_events.py:586: in run_forever
    self._check_running()
[...]
E           RuntimeError: This event loop is already running

[blink1073]

No, this one: https://github.com/jupyter/jupyter_client/blob/1aa276b84c53e269dce309bb5cefaaf9bd14f8f6/jupyter_client/utils.py#L51

[vidartf]

@kevin-bates I've fixed things up now (re-added validator using run_sync, fixed tests), and also added back the default value of the ServerApp.preferred_dir, so that it will be backwards compatible with older versions of jupyterlab-server.

[kevin-bates]

Thank you @vidartf - definitely one of those "easier said than done" PRs!

[vidartf]

@kevin-bates I changed to not having a leading / for preferred dir, and also made the file contents manager accept OS paths (validator will coerce to API path). With 2.0.0 released, now might be a good time to merge if the changes look good 😉

[blink1073]

Heads up, this PR picked up a couple conflicts from the linting refactor in #1114

[kevin-bates]

Thanks for the great work here @vidartf - merging.

[vidartf]

@meeseeksdev Please backport to 1.x

[kevin-bates]

I guess I was a little surprised to see this backport request since it introduces a deprecation. As a result, this warning message may be a little confusing to folks running 1.x.

ServerApp.preferred_dir config is deprecated in jupyter-server 2.0. Use FileContentsManager.preferred_dir instead

Had we known this was to be backported this warning could have been worded...

ServerApp.preferred_dir config is deprecated and will be removed in jupyter-server 3.0. Use FileContentsManager.preferred_dir instead

That said, I don't think this is worth the fuss to change - but perhaps something to consider down the road.

[blink1073]

@vidartf do we need to revert this prior to making a patch release, since it seems to depend on jupyterlab/jupyterlab_server#324?

[vidartf]

We do not strictly need it (nothing will fail directly), but it might be cleaner. If we release this as-is, people that use the preferred_dir config will get a FutureWarning, and if they follow that advice, the setting will be ignored (not correctly passed to JupyterLab).

[blink1073]

This to me indicates that we should either depend on the unreleased jupyterlab_server version, or add a feature guard before giving that advice.

[fcollonval]

jupyterlab_server 2.18.0 has been released with this feature