MNT: Replace some distutils imports with setuptools by pllim · Pull Request #10571 · astropy/astropy

pllim · 2020-07-15T20:37:50Z

Update

This follows up on #10576 to replace some distutils imports with setuptools. This should now be uncontroversial but it does not completely remove the warning that #10576 is ignoring.

Also see #10578 for the future.

Original Description

~~This pull request is to address~~ this error that started showing up during test collection in CI.

==================================== ERRORS ====================================
_ ERROR collecting .tox/py37-test/lib/python3.7/site-packages/astropy/tests/command.py _
../../.tox/py37-test/lib/python3.7/site-packages/astropy/tests/command.py:16: in <module>
    from setuptools import Command
../../.tox/py37-test/lib/python3.7/site-packages/setuptools/__init__.py:7: in <module>
    import setuptools.distutils_patch  # noqa: F401
../../.tox/py37-test/lib/python3.7/site-packages/setuptools/distutils_patch.py:59: in <module>
    warn_distutils_present()
../../.tox/py37-test/lib/python3.7/site-packages/setuptools/distutils_patch.py:26: in warn_distutils_present
    "Distutils was imported before Setuptools. This usage is discouraged "
E   UserWarning: Distutils was imported before Setuptools. This usage is discouraged and may exhibit undesirable behaviors or errors. Please use Setuptools' objects directly or at least import Setuptools first.

I think this is due to new setuptools release 49.2.0 but I am not 100% sure.

astropy/__init__.py

saimn · 2020-07-15T23:48:46Z

Yes, seems related to recent changes in setuptools:

v49.2.0
    #2230: Now warn the user when setuptools is imported after distutils modules have been loaded 
(exempting PyPy for 3.6), directing the users of packages to import setuptools first.

But the real motivation for this is a bit older:

v48.0.0
    #2143: Setuptools adopts distutils from the Python 3.9 standard library and no longer depends on distutils
 in the standard library. When importing setuptools or setuptools.distutils_patch, Setuptools will expose its bundled 
version as a top-level distutils package (and unload any previously-imported top-level distutils package), retaining 
the expectation that distutils' objects are actually Setuptools objects. To avoid getting any legacy behavior from the
 standard library, projects are advised to always "import setuptools" prior to importing anything from distutils. 
This behavior happens by default when using pip install or pep517.build. Workflows that rely on setup.py (anything) 
will need to first ensure setuptools is imported. One way to achieve this behavior without modifying code is to invoke
 Python thus: python -c "import setuptools; exec(open('setup.py').read())" (anything).

https://github.com/pypa/setuptools/blob/master/CHANGES.rst

All of this makes sense when executing setup.py and with the recent changes in the packaging ecosystem, but I guess that also means that we should really avoid using setuptools in the package itself.

astrofrog · 2020-07-16T09:53:47Z

Instead of importing at the top level, which would add a runtime dependency on setuptools, I think we should fix any of the distutils imports in setup_package.py files to just be setuptools imports instead, so:

from distutils.core import Extension                                                                                                               
from disutils.dep_util import newer_group

would become:

from setuptools import Extension                                                                                                               
from setuptools.dep_util import newer_group

I think there is one instance not in a setup_package.py file which is:

https://github.com/astropy/astropy/blob/7a81d70882d9797859954134489a5c802a067606/astropy/wcs/tests/extension/setup.py

and I think for that it's also safe to use setuptools (if needed, the test using it - if any - can be made to be conditional on setuptools being installed).

Basically we should just not have any direct distutils imports.

pllim · 2020-07-16T19:41:38Z

Not sure how to get rid of usage of from distutils.version import LooseVersion and I am not really sure what exactly is triggering the error, as it keeps getting stuck at from setuptools import Command.

pllim · 2020-07-16T19:45:23Z

tl;dr -- Just fixing distutils in setup_package.py does not seem to fix the problem. Is it time we get rid of the test runner?

mhvk · 2020-07-17T13:29:53Z

Numpy ended up pinning the version: numpy/numpy#16870. From the discussion at pypa/setuptools#2261, it would seem best to do as @astrofrog noted, try to use setuptools throughout. I'm not against replacing LooseVersion with a simpler version - we seem to need it only for really simple version strings (like numpy's).

p.s. I still like the idea of the package providing a way to test itself

pllim · 2020-07-17T15:58:52Z

Thanks for the links, @mhvk ! Given pypa/setuptools#2261 (comment), maybe we want to wait a bit before going forward with this PR.

In the meantime, I think @jdavies-st 's suggestion to whitelist is a very good one -- see #10576 . (Sorry for the inconvenience caused, James!)

p.s. I avoided pinning setuptools like Numpy because pinning a max version had bitten us in the past.

pllim · 2020-07-17T18:09:47Z

Update: I reduced the scope of this PR. Should now be uncontroversial. There are still some leftover distutils imports that we can worry about later.

./astropy/io/misc/asdf/tags/transform/tests/test_transform.py:from distutils.version import LooseVersion
./astropy/io/votable/util.py:from distutils import version
./astropy/tests/helper.py:from distutils.version import LooseVersion
./astropy/tests/command.py:from distutils import log
./astropy/utils/introspection.py:from distutils.version import LooseVersion
./astropy/visualization/scripts/fits2bitmap.py:from distutils.version import LooseVersion
./astropy/visualization/tests/test_norm.py:from distutils.version import LooseVersion
./astropy/visualization/wcsaxes/tests/test_misc.py:from distutils.version import LooseVersion
./astropy/wcs/tests/extension/setup.py:from distutils.core import setup, Extension
./astropy/wcs/tests/test_wcs.py:from distutils.version import LooseVersion
./astropy/wcs/tests/test_wcsprm.py:from distutils.version import LooseVersion
./astropy/wcs/wcsapi/tests/test_fitswcs.py:from distutils.version import LooseVersion
./docs/nitpick-exceptions:py:obj distutils.version.LooseVersion

pllim · 2020-07-17T18:14:56Z

Oh weird. What I thought was uncontroversial made test_wcsapi_extension fail: https://travis-ci.org/github/astropy/astropy/jobs/709276303

mhvk

Looks good to me. Ideally, we'd have working links to setuptools.Extension (see in-line), and a suggestion to get rid of log. But neither are very important, so approving.

mhvk · 2020-07-17T18:28:20Z

docs/development/building.rst

 anywhere in the package, and then looks for a function called ``get_extensions``
 inside each of these files. This function should return a list of
-:class:`distutils.core.Extension` objects, and these are combined into an
+``setuptools.Extension`` objects, and these are combined into an


Would be good to fix this to point to the right documentation. Would adding an intersphinx_mapping['setuptools'] to docs/conf.py help?

Ops, this is already merged. See #10579 😸

mhvk · 2020-07-17T18:41:22Z

astropy/tests/command.py

 from contextlib import contextmanager

 from setuptools import Command
+from distutils import log


Since it is basically not used except to give some info messages, I think it can be either replaced with import logging as log or the commands replaced with print. In favour of the earlier option is:
https://github.com/python/cpython/blob/164b04c47e61bd35d55e61bc74f9fd646eba81bb/Lib/distutils/log.py#L3-L4

I'll leave this as future exercise but I'll look into intersphinx. Thanks for the review!

jdavies-st · 2020-07-17T19:16:14Z

Not sure how to get rid of usage of from distutils.version import LooseVersion

One can probably use packaging.version.parse, but that will be a new dependency on packaging, though packaging is also vendorized in setuptools. But it will be good to do that because 1) distutils LooseVersion is undocumented, and 2) it does not implement that latests PEPs that deal with version handling for python (PEP 440).

mwcraig · 2020-07-17T19:35:47Z

But it will be good to do that because 1) distutils LooseVersion is undocumented, and 2) it does not implement that latests PEPs that deal with version handling (PEP 440).

Yes, this is...interesting: https://github.com/astropy/astropy/blob/master/astropy/utils/introspection.py#L144

My inclination is to use the vendored version of packaging from setuptools to avoid the extra dependency.

Shall I open a separate PR to do that?

pllim · 2020-07-17T19:38:12Z

Shall I open a separate PR to do that?

Yes, please. 😉

mwcraig · 2020-07-17T19:47:12Z

ok, I'm going to go ahead and merge this since it has been approved. Thanks @pllim!

pllim · 2020-07-17T20:02:56Z

While LooseVersion might have a way forward, test still fails when this is replaced with setuptools. No idea what's going on here:

astropy/astropy/wcs/tests/extension/setup.py

Line 14 in 25ac198

from distutils.core import setup, Extension

See #10581

mhvk · 2020-07-17T21:24:45Z

Note: one advantage of LooseVersion is that it is from the standard library - we currently do not require setuptools as a run-time dependency and I think just version checks are not quite worth adding it. (People who use astropy via a distribution or conda won't have setuptools by default.) Main question is whether we ever need more than __version__.split('.') (or even straight string comparison...)

pllim · 2020-07-17T23:56:07Z

FWIW, maybe the reply at pypa/setuptools#2261 (comment) is helpful.

mwcraig · 2020-07-20T14:17:42Z

Note: one advantage of LooseVersion is that it is from the standard library - we currently do not require setuptools as a run-time dependency and I think just version checks are not quite worth adding it.

It is odd that something like this is not part of the standard library. I think many of the places it is used it is in files that are part of the test suite; adding setuptools or packaging itself as a dependency seems less burdensome there.

In any event, I'll open a PR later today for further (and more concrete) discussion.

MNT: Replace some distutils imports with setuptools

eteq · 2020-07-29T15:38:32Z

@pllim - this turns out to be pretty complicated of a backport to v4.0.x . Before trying to do it, I want to confirm: Does it really need to be in 4.0.x ? I'm a bit worried of unintended consequences in things like logging that might be surprising in an LTS...

pllim · 2020-07-29T15:43:03Z

Yeah, okay, no need to backport then, as long as this below is in 4.0.x, should be fine until Python org really pulls the plug but I doubt they would do it so soon.

astropy/setup.cfg

Line 112 in d65dabb

ignore:Distutils was imported before Setuptools

pllim · 2020-07-29T15:43:40Z

p.s. I'll leave it for you to re-milestone. Sorry for the trouble, @eteq !

eteq · 2020-07-29T15:44:39Z

Sorry for the trouble, @eteq !

No trouble at all!

pllim added testing no-changelog-entry-needed labels Jul 15, 2020

pllim requested a review from astrofrog July 15, 2020 20:37

pllim added this to the v4.0.2 milestone Jul 15, 2020

pllim added the Bug label Jul 15, 2020

This comment has been minimized.

Sign in to view

pllim force-pushed the distutils-import branch from 2e5828c to 8b400fa Compare July 15, 2020 21:14

pllim commented Jul 15, 2020

View reviewed changes

astropy/__init__.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

pllim added the ¯\_(ツ)_/¯ label Jul 15, 2020

pllim force-pushed the distutils-import branch from 8b400fa to bd4a2cf Compare July 16, 2020 19:40

mhvk mentioned this pull request Jul 17, 2020

Use system libraries on apt #10555

Merged

pllim mentioned this pull request Jul 17, 2020

MNT: Temporarily ignore setuptools warning about distutils #10576

Merged

MNT: Replace some distutils imports with setuptools

02e24ee

pllim force-pushed the distutils-import branch from bd4a2cf to 02e24ee Compare July 17, 2020 18:01

pllim changed the title ~~MNT: Import distutils after setuptools~~ MNT: Replace some distutils imports with setuptools Jul 17, 2020

pllim added installation and removed Bug ¯\_(ツ)_/¯ testing labels Jul 17, 2020

Looks like distutils cannot be replaced here.

eeaa7c9

pllim mentioned this pull request Jul 17, 2020

MNT: Un-ignore Distutils was imported before Setuptools #10578

Closed

mhvk approved these changes Jul 17, 2020

View reviewed changes

mwcraig merged commit 25ac198 into astropy:master Jul 17, 2020

pllim deleted the distutils-import branch July 17, 2020 19:48

pllim mentioned this pull request Jul 17, 2020

DOC: Add setuptools intersphinx #10579

Closed

This was referenced Jul 17, 2020

MNT: Replace distutils.log with astropy logger #10580

Merged

MNT: Replace distutils with setuptools in wcs extension test #10581

Merged

eteq pushed a commit that referenced this pull request Jul 28, 2020

Merge pull request #10571 from pllim/distutils-import

2206b2e

MNT: Replace some distutils imports with setuptools

eteq modified the milestones: v4.0.2, v4.1.1 Jul 29, 2020

bsipocz modified the milestones: v4.1.1, v4.2, v4.1 Nov 25, 2020

Uh oh!

Conversation

pllim commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update

Original Description

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

saimn commented Jul 15, 2020

Uh oh!

astrofrog commented Jul 16, 2020

Uh oh!

pllim commented Jul 16, 2020

Uh oh!

pllim commented Jul 16, 2020

Uh oh!

mhvk commented Jul 17, 2020

Uh oh!

pllim commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Jul 17, 2020

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

pllim Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

mhvk Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

pllim Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

pllim Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

jdavies-st commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwcraig commented Jul 17, 2020

Uh oh!

pllim commented Jul 17, 2020

Uh oh!

mwcraig commented Jul 17, 2020

Uh oh!

pllim commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jul 17, 2020

Uh oh!

pllim commented Jul 17, 2020

Uh oh!

mwcraig commented Jul 20, 2020

Uh oh!

eteq commented Jul 29, 2020

Uh oh!

pllim commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Jul 29, 2020

Uh oh!

eteq commented Jul 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pllim commented Jul 15, 2020 •

edited

Loading

pllim commented Jul 17, 2020 •

edited

Loading

pllim commented Jul 17, 2020 •

edited

Loading

jdavies-st commented Jul 17, 2020 •

edited

Loading

pllim commented Jul 17, 2020 •

edited

Loading

pllim commented Jul 29, 2020 •

edited

Loading