-
Notifications
You must be signed in to change notification settings - Fork 445
Switch from entrypoints to importlib.metadata #792
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #792 +/- ##
==========================================
- Coverage 91.54% 91.01% -0.54%
==========================================
Files 17 17
Lines 1621 1636 +15
==========================================
+ Hits 1484 1489 +5
- Misses 137 147 +10
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
MSeal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one build failure looks unrelated result of merging master in. I'll take a look later at it to resolve if you don't get it before me. Thanks for posting the improvement!
d100d5f to
b41914f
Compare
b41914f to
30394a4
Compare
|
@Borda I've corrected the docstring you've pointed out (sorry it took me so long!) |
|
|
||
| include .bumpversion.cfg | ||
|
|
||
| include papermill/tests/fixtures/foo-0.0.1.dist-info/METADATA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks strange to be included, is it testing artifact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a testing file -- we provide just enough wheel metadata in that path to give us one entry point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Borda any opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that test shall not be distributed, so for testing you can install it with -e so if shall see all local files too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Borda It literally caused a GitHub Actions failure when I didn't include it, I'll note.
|
I'm looking forward to it as it allows me to drop the entrypoints package from Fedora Linux. Is there any plan to merge and release this? |
|
You might want to remove entrypoints also from the requirements/docs.txt file: papermill/requirements/docs.txt Line 10 in 5384731
|
Since the minimum Python version supported now directly supports querying entry points using the standard library, write a wrapper around to support both upstream APIs, and make use of it, rather than the external entrypoints package.
30394a4 to
762313b
Compare
|
Is there any plan to move this forward please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates from the external entrypoints package to the standard library's importlib.metadata module for querying entry points, as Python 3.8+ (the minimum supported version) includes this functionality natively.
Key changes:
- Added a compatibility wrapper
get_entrypoints_group()to support both Python 3.8/3.9 (dictionary-based API) and Python 3.10+ (select-based API) - Removed
entrypointsdependency from requirements files - Updated all usages of
entrypoints.get_group_all()to use the new wrapper function
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| papermill/utils.py | Adds the get_entrypoints_group() wrapper function and imports entry_points from importlib.metadata |
| papermill/iorw.py | Updates to use the new wrapper function and removes entrypoints import |
| papermill/engines.py | Updates to use the new wrapper function and removes entrypoints import |
| papermill/tests/test_utils.py | Adds test coverage for the new wrapper function using test fixtures |
| papermill/tests/test_iorw.py | Updates test mocks to patch entry_points instead of entrypoints.get_group_all |
| papermill/tests/test_engines.py | Updates test mocks to patch entry_points instead of entrypoints.get_group_all |
| papermill/tests/fixtures/foo-0.0.1.dist-info/entry_points.txt | Creates fixture entry points metadata for testing |
| papermill/tests/fixtures/foo-0.0.1.dist-info/METADATA | Creates fixture package metadata for testing |
| requirements.txt | Removes entrypoints dependency |
| requirements/docs.txt | Removes entrypoints dependency |
| MANIFEST.in | Includes the new test fixtures in the distribution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # We need to cast to a list here, 3.8/3.9 and 3.10+ return different | ||
| # types. | ||
| eps = list(get_entrypoints_group("papermill.tests.fake")) | ||
| sys.path.pop() | ||
| assert eps[0].name == "foo" |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test modifies sys.path but doesn't properly restore it in case of test failure. If an assertion fails before sys.path.pop(), the modified path will persist. Consider using a try-finally block or a context manager to ensure cleanup:
sys.path.insert(0, Path(__file__).parent / "fixtures")
try:
eps = list(get_entrypoints_group("papermill.tests.fake"))
assert eps[0].name == "foo"
finally:
sys.path.pop(0)| # We need to cast to a list here, 3.8/3.9 and 3.10+ return different | |
| # types. | |
| eps = list(get_entrypoints_group("papermill.tests.fake")) | |
| sys.path.pop() | |
| assert eps[0].name == "foo" | |
| try: | |
| # We need to cast to a list here, 3.8/3.9 and 3.10+ return different | |
| # types. | |
| eps = list(get_entrypoints_group("papermill.tests.fake")) | |
| assert eps[0].name == "foo" | |
| finally: | |
| sys.path.pop(0) |
| # We need to cast to a list here, 3.8/3.9 and 3.10+ return different | ||
| # types. | ||
| eps = list(get_entrypoints_group("papermill.tests.fake")) | ||
| sys.path.pop() |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sys.path.pop() call will remove the last element instead of the first element that was inserted. This should be sys.path.pop(0) to properly clean up the modification made by sys.path.insert(0, ...) on line 68.
| sys.path.pop() | |
| sys.path.pop(0) |
Since the minimum Python version supported now directly supports querying entry points using the standard library, write a wrapper around to support both upstream APIs, and make use of it, rather than the external entrypoints package.