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

EntryPoints compatibility #323

Merged
merged 4 commits into from May 31, 2021
Merged

EntryPoints compatibility #323

merged 4 commits into from May 31, 2021

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented May 30, 2021

As described in #300, there are use-cases for the result of Distribution.entry_points that expect a mutable, list-like object with index access. These changes re-enable those behaviors while also publishing a Deprecation warning when mutable operations are enacted.

@jaraco
Copy link
Member Author

jaraco commented May 30, 2021

@asottile I believe this change addresses the compatibility concerns raised in #300 and associated conversations. Would you have a look and let me know if it satisfies the use-cases you've encountered?

@asottile
Copy link
Contributor

it doesn't, this only reintroduces deprecated apis with no replacement

@jaraco
Copy link
Member Author

jaraco commented May 30, 2021

I'm sorry. I thought the replacement was pretty obvious from the deprecation warnings. Here's how one can adapt to the future behavior for index access:

importlib_metadata bugfix/300-entry-points-by-index $ .tox/python/bin/python -Wd -c "import importlib_metadata as md; print(md.distribution('pip').entry_points[0])"
EntryPoint(name='pip', value='pip._internal.cli.main:main', group='console_scripts')
<string>:1: DeprecationWarning: Accessing entry points by index is deprecated. Cast to tuple if needed.
importlib_metadata bugfix/300-entry-points-by-index $ .tox/python/bin/python -Wd -c "import importlib_metadata as md; print(tuple(md.distribution('pip').entry_points)[0])"
EntryPoint(name='pip', value='pip._internal.cli.main:main', group='console_scripts')

And for sorting:

importlib_metadata bugfix/300-entry-points-by-index $ .tox/python/bin/python -Wd -c "import importlib_metadata as md; eps = md.distribution('pip').entry_points; eps.sort(); print(eps[0].name)"
pip
<string>:1: DeprecationWarning: EntryPoints list interface is deprecated. Cast to list if needed.
<string>:1: DeprecationWarning: Accessing entry points by index is deprecated. Cast to tuple if needed.
importlib_metadata bugfix/300-entry-points-by-index $ .tox/python/bin/python -Wd -c "import importlib_metadata as md; eps = list(md.distribution('pip').entry_points); eps.sort(); print(eps[0].name)"
pip
# even better - prefer 'sorted' to sort in place:
importlib_metadata bugfix/300-entry-points-by-index $ .tox/python/bin/python -Wd -c "import importlib_metadata as md; eps = sorted(md.distribution('pip').entry_points); print(eps[0].name)"
pip

Historically, I've put replacements like this in the changelog. Would that suffice?

@asottile
Copy link
Contributor

asottile commented May 30, 2021

both of those are significantly worse than the apis being replaced, O(1) -> O(n) copy + O(n) space + O(1), in-place sort to O(N) space

@jaraco
Copy link
Member Author

jaraco commented May 31, 2021

Acknowledged, the performance is sub-optimal. In my experience, the cost of O(n) operations is often negligible for n < 10000. I'm unsure if the added cost here is a practical concern or not, but I suspect it's a bearable cost. I do wish to reduce the dependence on mutable types, but leave open the possibility of a downstream consumer owning that behavior if they wish.

I'd ultimately like to make entry_points() return a generator to minimize the immediate cost of the call and to enable efficient use-cases like the one you describe and perhaps others. I chose not to make that optimization in this round due as I was unaware of any use-cases that demanded it and the implementation details seemed non-trivial given other goals (simple re-usability of the result).

For now, I propose to either (a) accept the performance penalty where practical, or (b) devise alternative approaches for these specialized use cases outside of the default interface.

It may make sense for EntryPoints to present a group_map or similar method that provides an O(1) lookup by group. I've seen one use case in virtualenv that may benefit from this optimization. Another approach may be to allow entry_points to accept a container parameter that would allow any caller to develop a specialized container for the iterable of EntryPoint objects. I welcome bug reports to describe the use-cases that would benefit from such a feature.

@jaraco jaraco merged commit 5fb7029 into main May 31, 2021
@jaraco jaraco deleted the bugfix/300-entry-points-by-index branch May 31, 2021 14:52
@asottile
Copy link
Contributor

the performance regression here is unacceptable. it causes code to be 10-100x slower

please actually consider feedback before paving over it -- you're not being empathetic to my asks

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

Successfully merging this pull request may close these issues.

None yet

2 participants