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

extensions: allow extensions in namespace packages #523

Conversation

oliver-sanders
Copy link
Contributor

@oliver-sanders oliver-sanders commented May 20, 2021

This allows Jupyter extensions to be developed in Python namespace packages.

Related to: #222 (comment)

See PEP420.

Currently Jupyter Server extensions developed in Python namespace packages fall over with some nasty traceback:

$ t_ns_jps_ext
[I 2021-05-19 12:40:39.457 ServerApp] t_namespace | extension was successfully linked.
Traceback (most recent call last):
  File ".../t_ns_jps_ext", line 33, in <module>
    sys.exit(load_entry_point('jps-namespace-extension', 'console_scripts', 't_ns_jps_ext')())
  File ".../python3.9/site-packages/jupyter_server/extension/application.py", line 518, in launch_instance
    serverapp = cls.initialize_server(argv=args)
  File ".../python3.9/site-packages/jupyter_server/extension/application.py", line 488, in initialize_server
    serverapp.initialize(
  File ".../python3.9/site-packages/traitlets/config/application.py", line 87, in inner
    return method(app, *args, **kwargs)
  File ".../python3.9/site-packages/jupyter_server/serverapp.py", line 1855, in initialize
    point = self.extension_manager.extension_points[starter_extension]
KeyError: 'test_namespace_extension'

Here's an example extension developed in a Python namespace package: https://github.com/oliver-sanders/jupyter-server-namespace-extension-test

@welcome
Copy link

welcome bot commented May 20, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@oliver-sanders oliver-sanders force-pushed the extensions-in-namespace-packages branch from f866135 to b3f101a Compare May 20, 2021 15:23
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2021

Codecov Report

Merging #523 (4a82af5) into master (f29bb11) will increase coverage by 0.04%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
+ Coverage   76.30%   76.34%   +0.04%     
==========================================
  Files         110      110              
  Lines        9814     9835      +21     
  Branches     1067     1069       +2     
==========================================
+ Hits         7489     7509      +20     
- Misses       1944     1946       +2     
+ Partials      381      380       -1     
Impacted Files Coverage Δ
jupyter_server/extension/application.py 71.63% <20.00%> (-1.05%) ⬇️
jupyter_server/tests/test_utils.py 100.00% <100.00%> (ø)
jupyter_server/utils.py 64.77% <100.00%> (+1.62%) ⬆️
jupyter_server/services/kernels/kernelmanager.py 80.79% <0.00%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f29bb11...4a82af5. Read the comment docs.

@Zsailer
Copy link
Member

Zsailer commented May 27, 2021

Thanks, @oliver-sanders. This is awesome!

This was briefly mentioned in the Jupyter Server meeting today. The "committers" around Jupyter Server have been a little low on bandwidth the past couple of weeks, so review has been a little slower than normal. You should start getting some review over the next week. 😎

@oliver-sanders
Copy link
Contributor Author

oliver-sanders commented Jun 2, 2021

Rebased and switched to a nicer way of detecting namespace packages by using the __file__ attribute, this is part of the Python language specification. Updated the PR description with explanation.

@oliver-sanders oliver-sanders marked this pull request as ready for review June 2, 2021 12:38
@Zsailer
Copy link
Member

Zsailer commented Jun 2, 2021

Thanks, @oliver-sanders. 🚀

I think this change is good as-is, even though we duplicate some importing in the ExtensionPoint API

self._module = importlib.import_module(self._module_name)
except ImportError:
raise ExtensionModuleNotFound(
"The submodule '{}' could not be found. Are you "
"sure the extension is installed?".format(self._module_name)
)

In the long term, I believe there is some work to do around extension distribution/packaging/discovery. The get_extension_package classmethod is pretty brittle. It’s particularly annoying that extensions loaded from the current working directory (i.e. not from site-packages; usually happens when you’re developing the extension) are given the name __main__ by the ExtensionManager. I just haven’t had a change to revisit this yet.

@blink1073 blink1073 added this to the 1.9 milestone Jun 2, 2021
@oliver-sanders oliver-sanders force-pushed the extensions-in-namespace-packages branch from fd55fe7 to a43b90c Compare June 7, 2021 16:15
@oliver-sanders oliver-sanders force-pushed the extensions-in-namespace-packages branch from a43b90c to 4a82af5 Compare June 8, 2021 10:01
@oliver-sanders
Copy link
Contributor Author

oliver-sanders commented Jun 9, 2021

Two One test failures which appears unrelated.

[edit] fixed in rerun
Warning: Failed to download action 
ERROR: Could not find a version that satisfies the requirement jupyter_packaging<2,~=0.9 (from versions: none)

@blink1073 blink1073 modified the milestones: 1.9, 1.10 Jun 24, 2021
@oliver-sanders
Copy link
Contributor Author

[update] This PR is hanging on:

  • Conclusion of the implementation discussion above
    • No especially nice solutions, I've outlined two options, need to pick one.
  • Likely unrelated documentation build failure
    • ERROR: No matching distribution found for jupyter_packaging<2,~=0.9

@blink1073
Copy link
Contributor

Hi @oliver-sanders, can you please see yesterday's meeting notes about this topic? Thanks again for your patience on this one!

@oliver-sanders
Copy link
Contributor Author

oliver-sanders commented Jul 2, 2021

@blink1073 sure, no problem, I'm happy to implement this however is desired.

Currently, PR relies on private APIs, which is concerning.

It's not ideal, but I don't think it's a big issue because this implementation is brittle (i.e. will fail if the internals change). So if we ran the CI test battery against the latest dev version of Python (currently 3.10-dev) we would get ample early warning to patch any issues that crop up.

Can we build a more "defensive" way to discover namespace packages?

  • Make get_namespace_package try a series of steps to find a NS package
  • Try importing the private standard lib API to inspect the namespace package (how it's currently implemented in the PR)
  • If those private APIs are not found (i.e. change in the future), fallback to trying to import the namespace package directly.
  • If that fails, bail gracefully.

It would appear there is no especially nice solution to this problem so it comes down to picking a nasty method and ensuring that it is well tested.

I don't think implementing two nasty methods would be better than implementing one. The failure of the first method (which should never happen) would be silently suppressed which is nasty. The second method (which would never be run unless the first fails which it shouldn't) then becomes dead code and a potential bug in waiting.

@Zsailer
Copy link
Member

Zsailer commented Jul 6, 2021

Ok, I’m convinced. 😄

Let’s stick with option 1 (as implemented). We can add Python latest dev version (3.10.x) to our Github actions in a separate PR, perhaps #541.

@oliver-sanders, this is great stuff. Thank you for many of your recent contributions. If you’re ever available to make it to our weekly Jupyter Server meetings, we’d love to meet you!

@oliver-sanders
Copy link
Contributor Author

Cheers, added 3.10-dev in #541, Linux/MacOs tests pass, Windows fails to pip install (waiting on 3.10 support for pywin32?). I'll try and pop by a meeting sometime to say hi.

@Zsailer Zsailer merged commit 81d46e3 into jupyter-server:master Jul 7, 2021
@welcome
Copy link

welcome bot commented Jul 7, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@oliver-sanders oliver-sanders deleted the extensions-in-namespace-packages branch July 7, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants