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.

Sign up for GitHub

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

setup: move importlib_metadata to setup.cfg #109

Closed
wants to merge 2 commits into from
Closed

setup: move importlib_metadata to setup.cfg #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94ff579
setup: move importlib_metadata to setup.cfg
tmorrell Oct 28, 2024
7898c6b
setup: remove pytest-invenio to make imports cleaner
tmorrell Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
[build-system]
requires = ["setuptools", "wheel", "babel>2.8", "importlib_metadata"]
requires = ["setuptools", "wheel", "babel>2.8"]
build-backend = "setuptools.build_meta"

6 changes: 5 additions & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,18 @@ zip_safe = False
install_requires =
six>=1.10
isbnlib>=3.10.8
importlib-metadata>=6.11.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be needed if we have Python >= 3.9:

~/invenio/modules/idutils > python --version
Python 3.9.16
~/invenio/modules/idutils > python
Python 3.9.16 (main, Oct 12 2023, 17:53:07)
[Clang 15.0.0 (clang-1500.0.40.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from importlib.metadata import entry_points
>>>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it is needed for python >= 3.12

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right. also for python >= 3.12 your line works. BUT the idutils is using importlib_metadata:

idutils/idutils/ext.py

Line 53 in 812f640

from importlib_metadata import entry_points
and not importlib.metadata

Python 3.12.7 (main, Oct  2 2024, 16:29:41) [GCC 12.2.1 20230304] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from importlib_metadata import entry_points
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'importlib_metadata'
>>>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean the better fix would be to change from importlib_metadata import entry_points to from importlib.metadata import entry_points the difference is _ vs .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using importlib.metadata is possible for python3.9 because it has been added with python3.8: https://docs.python.org/3/library/importlib.metadata.html

Copy link
Contributor

@ntarocco ntarocco Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you have answered yourself 🤣

I asked @ptamarit if he could create a PR with:

  • changing the imports to importlib.metadata
  • changing the publish CI to use Python 3.9
  • remove any install dependency of importlib
    and notify everyone, but @tmorrell was too fast :)

I believe the above should work and can be applied to other modules when touched/needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried importlib.metadata and it didn’t work exactly the same. That’s not to say it couldn’t be used…but it’s not a drop in replacement

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried importlib.metadata and it didn’t work exactly the same. That’s not to say it couldn’t be used…but it’s not a drop in replacement

Yes you might be right. The same change was done here, and tested it.
Is it probably this one to apply?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s still using the dash version. I think it was an issue with the group parameter changing in importlib.metadata.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the doc, the root cause of the issue is here:

Changed in version 3.12: The “selectable” entry points were introduced in importlib_metadata 3.6 and Python 3.10. Prior to those changes, entry_points accepted no parameters and always returned a dictionary of entry points, keyed by group. With importlib_metadata 5.0 and Python 3.12, entry_points always returns an EntryPoints object. See backports.entry_points_selectable for compatibility options.

They did quite a mess here :(

I created another PRs with your commits and a bit of extra: #110


[options.extras_require]
tests =
pytest-black-ng>=0.4.0
pytest-cache>=1.0
pytest-runner>=2.6.2
pytest-invenio>=1.4.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to implement locally in the module the mocking of entrypoints so that the tests will pass

sphinx>=4.5
check-manifest>=0.42
pytest-isort>=3.0.0
pytest-pydocstyle>=2.2.3
pytest-cov>=3.0.0
# Kept for backwards compatibility
docs =

Expand Down
Loading