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

Drop extension_name for name in ExtensionApp #224

Closed
Zsailer opened this issue May 7, 2020 · 32 comments
Closed

Drop extension_name for name in ExtensionApp #224

Zsailer opened this issue May 7, 2020 · 32 comments

Comments

@Zsailer
Copy link
Member

Zsailer commented May 7, 2020

This issue is to track the change from ExtensionApp.extension_name to ExtensionApp.name

Prior discussion from #222.

Why the change from name -> extension_name for the config file path? Was this to avoid the legacy path resolution?

honestly, I don't have a good answer for that 😆. It was a change I made early on in the development of ExtensionApp's to avoid legacy path resolution, but (I think) I've since patched the places where that would be an issue.

It probably makes sense to switch back...

@blink1073
Copy link
Contributor

blink1073 commented May 7, 2020

For more context, we brought up some issues with the path resolution in a JEP Issue.

In JupyterLab, we have some robust logic for detecting whether user or sys-prefix should take precedence. If using a different name allows us to use better semantics then I am all for it 😄

@blink1073
Copy link
Contributor

blink1073 commented May 11, 2020

Another data point is that the PyPA recommends only shipping data files within the package itself: https://setuptools.readthedocs.io/en/latest/setuptools.html#non-package-data-files. Also, data_files do not get moved into the appropriate place during an editable install, and that is a won't fix.

The pattern I would like to see is:

  • Entry points used for discovery of extensions instead of having them insert themselves into a config.d directory.
  • Incorporation of the hierarchy detection logic for root/sys-prefix/user

@blink1073
Copy link
Contributor

cc @SylvainCorlay

@blink1073
Copy link
Contributor

blink1073 commented May 11, 2020

Adopting entry_points would also mean that we can avoid using magic-named functions like _jupyter_server_extension_paths.

@Zsailer
Copy link
Member Author

Zsailer commented May 11, 2020

The pattern I would like to see is:

  • Entry points used for discovery of extensions instead of having them insert themselves into a config.d directory.

How do you handle enabling/disabling when using the entry point mechanism? Can you store dynamic values (i.e. enabled = True/False) in an entry point plugin?

@Zsailer
Copy link
Member Author

Zsailer commented May 11, 2020

Adopting entry_points would also mean that we can avoid using magic-named functions like _jupyter_server_extension_paths.

This would be awesome. I need to do better familiarize myself with the entry point framework.

@blink1073
Copy link
Contributor

How do you handle enabling/disabling when using the entry point mechanism? Can you store dynamic values (i.e. enabled = True/False) in an entry point plugin?

The entry point would serve as a "soft" enable, that could be overridden by config.

@blink1073
Copy link
Contributor

@Zsailer
Copy link
Member Author

Zsailer commented May 14, 2020

Thanks, @blink1073. This is great!

We discussed this in the Jupyter Server meeting this morning. I'm going to draft a PR using the entry points.

@blink1073
Copy link
Contributor

Yeah I saw the meeting notes. 😉

@SylvainCorlay
Copy link
Contributor

SylvainCorlay commented May 15, 2020

Thanks for the ping @blink1073,

Entry points are interesting, although why are configuration directories to be avoided? Also, configuration directories are the default in most of Jupyter, and are language agnostic (which matters for other cases than server extensions such as kernels), so I am generally in favor of placing configuration and data files respectively in etc and share, the Unix way...

Regarding the change of APIs with respect to server extension, another change in jupyter_server is the renaming from load_jupyter_server_extension to load_jupyter_server_extension...

@Zsailer
Copy link
Member Author

Zsailer commented May 15, 2020

Regarding the change of APIs with respect to server extension, another change in jupyter_server is the renaming from load_jupyter_server_extension to _load_jupyter_server_extension...

Right, but jupyter server currently handles both. See this chunk:

https://github.com/jupyter/jupyter_server/blob/8dde7a63c0e99525431df99f7b5d5efaa081eb16/jupyter_server/extension/serverextension.py#L144-L154

@blink1073
Copy link
Contributor

@SylvainCorlay, I explained why not to prefer data_files in #224 (comment). My suggestion is to offer both conf.d and entry points, but not use data_files.

@vidartf
Copy link
Member

vidartf commented May 22, 2020

@blink1073 I don't think I understand the proposal fully: If we support conf.d, won't extensions be free to use data_files to configure their extension if they want to? If so, is the suggestion to only stop using data_files in official jupyter extensions? Or am I misunderstanding something further up the chain?

Similarly to @SylvainCorlay: Is your concern that your extensions will lose the ability to use data_files to configure your stuff, or is your concern the inability to gather and resolve the full configuration from both conf.d + data_files from a non-python system?

@blink1073
Copy link
Contributor

Sylvain and I spoke afterward, but to clarify here:

  • I am proposing that we allow either conf.d or entry points
  • An entry point is a "soft enable" that can be overridden by conf.d or other config

The overall point being that data_files is brittle, not well supported by PyPA and is not guaranteed to work in the future.

@SylvainCorlay
Copy link
Contributor

I think that it is a very good practice in general to keep making use of PREFIX/etc/jupyter/ (config) and PREFIX/etc/jupyter (data) and continue using the precedence order between USER > INSTALL_PREFIX > SYSTEM for extensions and configuration. Together with the conf.d approach it is very much the Unix way of doing things, and easy to browse without making use of dedicated tools to list different types of extension for a package.

For cases like e.g. kernels, which is language it is important to keep that mechanism anyways, because entry points are a python thing, so why not use the same structure for extension apps?

We cannot access INSTALL_PREFIX/share or INSTALL_PREFIX/etc without making use of data_files, and it is so much a common practice for any software package do use the content of these directories that the recommendation of PyPI to not use data files that are not in the package seems misguided in my opinion.

@blink1073
Copy link
Contributor

I'm not arguing that data_files aren't a good idea. I am arguing for practicality in that their support is not well documented or supported. I have been bitten many times by the fact that they aren't handled in development installs, and the syntax for using them is awkward at best, even with our helper functions.

The order of path preference I'll take up separately, since it was discussed at this week's server meeting as a possible JEP topic.

@SylvainCorlay
Copy link
Contributor

SylvainCorlay commented May 22, 2020

they aren't handled in development installs

That is indeed an issue, but there are major benefits. I find that having a directory structure in the source similar to that of the installation prefix like in xeus-python or nbconvert 6 makes reading the structure of the package easier.

@blink1073
Copy link
Contributor

I'm saying support both. I personally would be happy to never use data_files again unless their level of support changes.

@blink1073
Copy link
Contributor

Alternative proposal (gym thoughts): we make data_files work in develop mode using jupyter_packaging and embrace pyproject.toml everywhere using that.

@blink1073
Copy link
Contributor

Cool, thanks for pushing back @SylvainCorlay. "There should be one-- and preferably only one --obvious way to do it."

@blink1073
Copy link
Contributor

cf jupyter/jupyter-packaging#34

@Zsailer
Copy link
Member Author

Zsailer commented Jun 11, 2020

This is great. Thanks, y'all.

Follow-up question—

Right now, we only use config.d for enabling/disabling server extensions. If we want to add support for discovering extensions (with soft enable) using config.d , we need extensions to add a config file in this directory that means "I'm installed but not (hard) enabled/disabled". This should be the same file that's affected when the user enables/disables to prevent multiple files for the same extension. This file needs an intermediate state that just says, "I'm installed".

What would such a file look like? Is it empty and we extract the extension name from its file name? Or do we need some defined JSON pattern?

@blink1073
Copy link
Contributor

I thought we were also using config.d to indicate installed? What constitutes "installed" for a current serverextension?

@Zsailer
Copy link
Member Author

Zsailer commented Jun 11, 2020

What constitutes "installed" for a current serverextension?

There isn't a "discovery" mechanism for installed extensions that are not explicitly enabled/disabled.

It the server extension module is listed somewhere in the jpserver_extensions trait, the server will try to import it and load it.

Otherwise, an extension has to create a JSON file with config enabling it for Jupyter Server to see it.

@Zsailer
Copy link
Member Author

Zsailer commented Jun 11, 2020

"Installed" to me means the extension python package is installed and Jupyter Server is aware it exists.

Right now, you cannot have an extension that is installed—thereby visible by jupyter server extension list—without explicitly enabling/disabling. There is no "soft enable" mode.

@vidartf
Copy link
Member

vidartf commented Jun 16, 2020

I thought we were also using config.d to indicate installed?

We do for nbextensions currently (by putting the static assets in the nbextensions share folder), but not server extensions.

@blink1073
Copy link
Contributor

blink1073 commented Jun 16, 2020

Wait, no, jupyterlab is installing itself as a server extension here.

@vidartf
Copy link
Member

vidartf commented Jun 17, 2020

Wait, no, jupyterlab is installing itself as a server extension here.

That is jupyterlab enabling itself, i.e. equivalent of running jupyter serverextension enable .... There is no jupyter serverextension install command.

@vidartf
Copy link
Member

vidartf commented Jun 17, 2020

PS: That might seem like just semantics, but it means there is no way for a server extension to make itself discoverable by jupyter serverextension list except by either explicit enabling or disabling it.

@blink1073
Copy link
Contributor

Closing since we're using shared_data in hatch now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants