Supervisor / supervisor

Supervisor process control system for Unix (supervisord)
http://supervisord.org
Other
8.34k stars 1.23k forks source link

Replace runtime dependency on setuptools with modern libraries #1578

Closed ofek closed 1 year ago

ofek commented 1 year ago

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

On Python 3.8+ no dependencies are required

ofek commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

It's happening now! https://github.com/pypa/setuptools/pull/3843

ofek commented 1 year ago

@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 https://github.com/Supervisor/supervisor/blob/a7cb60d58b5eb610feb76c675208f87501d4bc4b/tox.ini#L33

mnaberez commented 1 year ago

It's happening now! https://github.com/pypa/setuptools/pull/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 0323a9ab2b8282994e95472cae1d78da7c3aa59d.

ofek commented 1 year ago

Fixed in https://github.com/Supervisor/supervisor/commit/0323a9ab2b8282994e95472cae1d78da7c3aa59d.

Nice! I rebased

mnaberez commented 1 year ago

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.