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

Convolution import error with setuptools v65.6.0 #14025

Closed
saimn opened this issue Nov 21, 2022 · 13 comments · Fixed by #14035
Closed

Convolution import error with setuptools v65.6.0 #14025

saimn opened this issue Nov 21, 2022 · 13 comments · Fixed by #14035
Labels
Milestone

Comments

@saimn
Copy link
Contributor

saimn commented Nov 21, 2022

setuptools v65.6.0 made a change in their bundled version of distutils, which is breaking the import of the compiled extension in convolution, which is done with numpy.ctypeslib.load_library (which depends on numpy.distutils).
Some of our builds are failing (currently RTD and wheels, but it seems the tox based one are still using the previous version of setuptools, probably because virtualenv did not update yet):

Traceback:
../astropy/astropy/test-sdist/lib/python3.11/site-packages/astropy/convolution/convolve.py:28: in <module>
    _convolve = load_library("_convolve", LIBRARY_PATH)
../astropy/astropy/test-sdist/lib/python3.11/site-packages/numpy/ctypeslib.py:137: in load_library
    from numpy.distutils.misc_util import get_shared_lib_extension
../astropy/astropy/test-sdist/lib/python3.11/site-packages/numpy/distutils/__init__.py:26: in <module>
    from . import ccompiler
../astropy/astropy/test-sdist/lib/python3.11/site-packages/numpy/distutils/ccompiler.py:20: in <module>
    from numpy.distutils import log
../astropy/astropy/test-sdist/lib/python3.11/site-packages/numpy/distutils/log.py:4: in <module>
    from distutils.log import Log as old_Log
E   ImportError: cannot import name 'Log' from 'distutils.log' (/home/runner/work/astropy/astropy/test-sdist/lib/python3.11/site-packages/setuptools/_distutils/log.py)

Ref numpy/numpy#22623, pypa/setuptools#3693

@saimn saimn added this to the v5.2 milestone Nov 21, 2022
@saimn saimn mentioned this issue Nov 21, 2022
10 tasks
@pllim
Copy link
Member

pllim commented Nov 21, 2022

This is affected downstream CIs left and right. We need to patch it up soon with something...

Even at RTD; example log: https://readthedocs.org/projects/jdaviz/builds/18694319/

[autosummary] generating autosummary for: ...

Traceback (most recent call last):
  File ".../astropy/convolution/convolve.py", line 28, in <module>
    _convolve = load_library("_convolve", LIBRARY_PATH)
  File ".../numpy/ctypeslib.py", line 137, in load_library
    from numpy.distutils.misc_util import get_shared_lib_extension
  File ".../numpy/distutils/__init__.py", line 26, in <module>
    from . import ccompiler
  File ".../numpy/distutils/ccompiler.py", line 20, in <module>
    from numpy.distutils import log
  File ".../numpy/distutils/log.py", line 4, in <module>
    from distutils.log import Log as old_Log
ImportError: cannot import name 'Log' from 'distutils.log' (.../setuptools/_distutils/log.py)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File ".../sphinx/events.py", line 94, in emit
    results.append(listener.handler(self.app, *args))
  File ".../sphinx_automodapi/automodsumm.py", line 257, in process_automodsumm_generation
    lines = automodsumm_to_autosummary_lines(sfn, app)
  File ".../sphinx_automodapi/automodsumm.py", line 329, in automodsumm_to_autosummary_lines
    filestr = automodapi.automodapi_replace(fr.read(), app, True, docname, False)
  File ".../sphinx_automodapi/automodapi.py", line 302, in automodapi_replace
    ispkg, hascls, hasfuncs, hasother, toskip = _mod_info(
  File ".../sphinx_automodapi/automodapi.py", line 415, in _mod_info
    for localnm, fqnm, obj in zip(*find_mod_objs(modname, onlylocals=onlylocals)):
  File ".../sphinx_automodapi/utils.py", line 81, in find_mod_objs
    __import__(modname)
  File ".../jdaviz/__init__.py", line 10, in <module>
    from jdaviz.app import *  # noqa
  File ".../jdaviz/app.py", line 42, in <module>
    from jdaviz.core.config import read_configuration, get_configuration
  File ".../jdaviz/core/__init__.py", line 2, in <module>
    import glue_astronomy.translators as _glue_astronomy_translators  # noqa
  File ".../glue_astronomy/translators/__init__.py", line 3, in <module>
    from . import spectral_cube  # noqa
  File ".../glue_astronomy/translators/spectral_cube.py", line 9, in <module>
    from spectral_cube import BooleanArrayMask
  File ".../spectral_cube/__init__.py", line 7, in <module>
    from .spectral_cube import (SpectralCube, VaryingResolutionSpectralCube)
  File ".../spectral_cube/spectral_cube.py", line 26, in <module>
    from astropy import convolution
  File ".../astropy/convolution/__init__.py", line 4, in <module>
    from .convolve import (convolve, convolve_fft, convolve_models,
  File ".../astropy/convolution/convolve.py", line 30, in <module>
    raise ImportError("Convolution C extension is missing. Try re-building astropy.")
ImportError: Convolution C extension is missing. Try re-building astropy.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File ".../sphinx/cmd/build.py", line 276, in build_main
    app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
  File ".../sphinx/application.py", line 262, in __init__
    self._init_builder()
  File ".../sphinx/application.py", line 335, in _init_builder
    self.events.emit('builder-inited')
  File ".../sphinx/events.py", line 105, in emit
    raise ExtensionError(__("Handler %r for event %r threw an exception") %
sphinx.errors.ExtensionError: Handler <function process_automodsumm_generation at ...>
for event 'builder-inited' threw an exception
(exception: Convolution C extension is missing. Try re-building astropy.)

@Cadair
Copy link
Member

Cadair commented Nov 21, 2022

Am I correct in the understanding that the long term answer he is that we need to stop using numpy.distutils ?

@Cadair
Copy link
Member

Cadair commented Nov 21, 2022

A short term answer may be to pin down setuptools everywhere?

@pllim
Copy link
Member

pllim commented Nov 21, 2022

Or maybe refactor that convolution code to not be C-extension? Or have it only emit warning on fail?

@saimn
Copy link
Contributor Author

saimn commented Nov 21, 2022

Pinning setuptools would mean adding it to our dependencies, not great. There are a few other solutions:

  • setuptools may revert their patch and deprecate distutils.log before removing it
  • numpy may move get_shared_lib_extension from numpy.distutils to maybe numpy.ctypeslib
  • we can wrap the C convolution extension with something else, replace ctypes with e.g. cython or a pure C extension (first one should be easy, second one requires a bit more work and knowledge)

@pllim
Copy link
Member

pllim commented Nov 21, 2022

I am going to try some dark magic I found at numpy/numpy#22623 (comment) and report back...

@saimn
Copy link
Contributor Author

saimn commented Nov 21, 2022

Some projects are only affected when installing. SETUPTOOLS_USE_DISTUTILS or pinning in pyproject.toml would work in this case. But we use numpy.ctypeslib.load_library at runtime, to load the C extension. So I don't think asking users to run their python with SETUPTOOLS_USE_DISTUTILS=stdlib is a good solution ;).

@pllim
Copy link
Member

pllim commented Nov 21, 2022

Just fixing RTD would be a nice first step... 😬

pllim added a commit to pllim/jdaviz that referenced this issue Nov 21, 2022
@pllim
Copy link
Member

pllim commented Nov 21, 2022

FWIW, pinning setuptools just under docs requirements alone is enough to make RTD happy, see spacetelescope/jdaviz#1861

@astrofrog
Copy link
Member

Or maybe refactor that convolution code to not be C-extension? Or have it only emit warning on fail?

We could also update the C extension to not be imported via ctypes but use a regular C extension instead as we do elsewhere (such as in io.fits or wcs)

@astrofrog
Copy link
Member

Ah or via cython as done in #14035

saimn added a commit to saimn/astropy that referenced this issue Nov 23, 2022
This is one way to fix astropy#14025, removing our dependency to
numpy.ctypeslib which itself relies on numpy.distutils
@saimn
Copy link
Contributor Author

saimn commented Nov 23, 2022

setuptools v65.6.1 has just been released, putting back a disutils.Log class with a deprecation warning:
https://setuptools.pypa.io/en/latest/history.html#v65-6-1

@pllim
Copy link
Member

pllim commented Nov 28, 2022

Thanks, all!

dougbrn pushed a commit to dougbrn/astropy that referenced this issue Mar 13, 2023
This is one way to fix astropy#14025, removing our dependency to
numpy.ctypeslib which itself relies on numpy.distutils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants