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

Replace runtime dependency on setuptools with modern libraries #1578

Merged
merged 2 commits into from
Mar 7, 2023

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Mar 2, 2023

This reduces the installation footprint and potential memory implications from merely importing it

On Python 3.8+ no dependencies are required

@ofek ofek force-pushed the setuptools branch 5 times, most recently from bb2c3ce to 2d4cb6f Compare March 2, 2023 21:50
@ofek
Copy link
Contributor Author

ofek commented Mar 2, 2023

Python 3.4 is so old that it would require custom logic for this. Are we dropping that soon? If not then I'll add the hacks, just please let me know

@mnaberez
Copy link
Member

mnaberez commented Mar 2, 2023

Python 3.4 is so old that it would require custom logic for this. Are we dropping that soon? If not then I'll add the hacks, just please let me know

There are no plans to drop any of the Python versions we currently support (see setup.py or CI for the list).

Reading https://setuptools.pypa.io/en/latest/pkg_resources.html

Use of pkg_resources is discouraged in favor of importlib.resources, importlib.metadata, and their backports (importlib_resources, importlib_metadata). Please consider using those libraries instead of pkg_resources.

Although it is "discouraged", use of pkg_resources is not deprecated. At least, it's not on the setuptools main branch as of today.

Reading the diff, this patch adds significant added installation complexity and maintenance burden. Multiple PyPI packages must be conditionally installed or not based on Python version. The Supervisor maintainers and the folks that repackage Supervisor for various distributions would have to deal with the fallout from that.

It's only on 3.9 or later that no additional packages are required. It would be great if supervisor could only require Python stdlib. If there is ever a version of Supervisor that only supports 3.9 or later, I'd absolutely apply some version of this patch (without the conditional shim stuff). However, I don't see that happening in the foreseeable future. Until then, or setuptools actually deprecates pkg_resources, I'm strongly leaning against all the added complexity here, considering how many Python environments already have setuptools.

@jaraco
Copy link
Contributor

jaraco commented Mar 3, 2023

Users are encouraged to move off pkg_reapurces asap and not wait for stdlib versions to sunset.

If the conditionality is a concern, perhaps consolidating the conditionality in a private module would help.

If conditional dependencies is a concern, the backports could be required unconditionally, a state that would be preferred over using setuptools/pkg_resources.

@mnaberez
Copy link
Member

mnaberez commented Mar 4, 2023

Users are encouraged to move off pkg_reapurces asap

After this issue was reported, I checked latest setuptools and pkg_resources is not deprecated on the main branch. If pkg_resources started emitting deprecation warnings, those would affect users and we would try to solve that for users. If you look at recent changelogs, the Supervisor project does actively remove deprecated functionality.

If conditional dependencies is a concern, the backports could be required unconditionally, a state that would be preferred over using setuptools/pkg_resources.

If we make no change here, then the users, maintainers, and people who repackage Supervisor do not have to deal with any changes and also have no risk of anything breaking unexpectedly by our changes. I think there should be a clear problem being solved (e.g. deprecation warnings) to warrant changing installation requirements, putting the backports into a private module that we'd have keep updated, etc.

@ofek
Copy link
Contributor Author

ofek commented Mar 4, 2023

Jason can confirm since he's a core maintainer of setuptools, but I think there is a plan to eventually emit deprecation warnings so it would be nice to do this before that even happens

@ofek
Copy link
Contributor Author

ofek commented Mar 5, 2023

It's happening now! pypa/setuptools#3843

@ofek ofek force-pushed the setuptools branch 4 times, most recently from d069bc8 to b8c0871 Compare March 5, 2023 18:45
@ofek
Copy link
Contributor Author

ofek commented Mar 5, 2023

@mnaberez everything is now passing except for a job that cannot find the right Python which is unrelated to this PR

edit: this should be 3.8 because that is what the CI sets up

basepython = python3.7

@mnaberez
Copy link
Member

mnaberez commented Mar 5, 2023

It's happening now! pypa/setuptools#3843

Setuptools 67.5.0 (2023-03-05) was released with a DeprecationWarning for pkg_resources. The next Supervisor release will include the patch to avoid the warning.

everything is now passing except for a job that cannot find the right Python which is unrelated to this PR

Fixed in 0323a9a.

@ofek
Copy link
Contributor Author

ofek commented Mar 6, 2023

Fixed in 0323a9a.

Nice! I rebased

@mnaberez mnaberez merged commit 8fe9410 into Supervisor:main Mar 7, 2023
@ofek ofek deleted the setuptools branch March 7, 2023 23:49
@mnaberez
Copy link
Member

mnaberez commented Mar 20, 2023

With these dependencies added to supervisor's setup.py in this pull request:

importlib-metadata; python_version < '3.8'
importlib-resources; python_version < '3.7'

I installed Supervisor in an empty virtual environment that had only pip and setuptools. I ran pip list --format=freeze before and after installing supervisor. These lists show the new packages that were installed (supervisor itself is not shown).

Python 2.7

configparser==4.0.2
contextlib2==0.6.0.post1
importlib-metadata==2.1.3
importlib-resources==3.3.1
pathlib2==2.3.7.post1
scandir==1.10.0
singledispatch==3.7.0
six==1.16.0
typing==3.10.0.0
zipp==1.2.0

Python 3.4

importlib-metadata==1.1.3
importlib-resources==1.0.2
pathlib2==2.3.7.post1
scandir==1.10.0
six==1.16.0
typing==3.10.0.0
zipp==1.2.0

Python 3.5

importlib-metadata==2.1.3
importlib-resources==3.2.1
zipp==1.2.0

Python 3.6

importlib-metadata==4.8.3
importlib-resources==5.4.0
typing-extensions==4.1.1
zipp==3.6.0

Python 3.7

importlib-metadata==6.0.0
typing_extensions==4.5.0
zipp==3.15.0

Python 3.8, 3.9, 3.10, 3.11
None.

Prior to the changes in this pull request, Supervisor depended only on setuptools. It required no additional dependencies on any of the above Python versions (2.7 - 3.11). We went from a project with 1 dependency, which is usually installed in every environment, to 10 in the worst case. Also concerning is the complexity of the dependency list. We went from 1 dependency (setuptools) on every version to a different list for every version of Python above.

Supervisor has a lot of non-Python users and should install easily on as many systems as possible, which is why it supports a lot of legacy Python versions and had no dependencies other than setuptools. I'm concerned that adding all these dependencies to Supervisor is going to cause installation failures.

It is great that on Python 3.8+ it is now possible to install Supervisor with 0 dependencies by using stdlib importlib instead of pkg_resources. Supervisor will keep using stdlib importlib on 3.8+. However, there does not seem to be a good solution for the older versions. A deprecation warning is better than potential installation failures, so we may need to use pkg_resources until a better solution is found.

mnaberez added a commit that referenced this pull request Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants