zeromq / pyzmq

PyZMQ: Python bindings for zeromq
http://zguide.zeromq.org/py:all
BSD 3-Clause "New" or "Revised" License
3.72k stars 636 forks source link

fix build_ext -j when pythran and numpy are present #1872

Closed mgorny closed 1 year ago

mgorny commented 1 year ago

Call the finalize_options() method of overridden distutils commands before running the configure command, in order to fix errors due to unconfigured --jobs option. This can be reproduced by running:

$ python setup.py build_ext -j12
[…]
error: '<' not supported between instances of 'str' and 'int'

Fatal: Falling back on bundled libzmq, but config has explicitly prohibited building the libzmq extension.
minrk commented 1 year ago

Looks like this causes Windows PyPy builds to fail with:

      File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\command\build_ext.py", line 247, in build_extension
        if ext._needs_stub:
    AttributeError: 'Extension' object has no attribute '_needs_stub'
full traceback ``` Traceback (most recent call last): File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-vfed2ggk\pp37-win_amd64\build\venv\site-packages\pip\_vendor\pep517\in_process\_in_process.py", line 363, in main() File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-vfed2ggk\pp37-win_amd64\build\venv\site-packages\pip\_vendor\pep517\in_process\_in_process.py", line 345, in main json_out['return_val'] = hook(**hook_input['kwargs']) File "C:\Users\runneradmin\AppData\Local\Temp\cibw-run-vfed2ggk\pp37-win_amd64\build\venv\site-packages\pip\_vendor\pep517\in_process\_in_process.py", line 262, in build_wheel metadata_directory) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\build_meta.py", line 417, in build_wheel wheel_directory, config_settings) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\build_meta.py", line 401, in _build_with_temp_dir self.run_setup() File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\build_meta.py", line 338, in run_setup exec(code, locals()) File "", line 1369, in File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\__init__.py", line 107, in setup return distutils.core.setup(**attrs) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\core.py", line 185, in setup return run_commands(dist) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\core.py", line 201, in run_commands dist.run_commands() File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\dist.py", line 969, in run_commands self.run_command(cmd) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\dist.py", line 1244, in run_command super().run_command(command) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\dist.py", line 988, in run_command cmd_obj.run() File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\wheel\bdist_wheel.py", line 343, in run self.run_command("build") File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\cmd.py", line 318, in run_command self.distribution.run_command(command) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\dist.py", line 1244, in run_command super().run_command(command) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\dist.py", line 988, in run_command cmd_obj.run() File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\command\build.py", line 131, in run self.run_command(cmd_name) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\cmd.py", line 318, in run_command self.distribution.run_command(command) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\dist.py", line 1244, in run_command super().run_command(command) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\dist.py", line 988, in run_command cmd_obj.run() File "C:\Users\runneradmin\AppData\Local\pypa\cibuildwheel\Cache\pypy3.7-v7.3.9-win64\lib_pypy\cffi\setuptools_ext.py", line 144, in run base_class.run(self) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\command\build_ext.py", line 84, in run _build_ext.run(self) File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\_distutils\command\build_ext.py", line 345, in run self.build_extensions() File "", line 1100, in build_extensions File "", line 1104, in build_extension File "C:\Users\RUNNER~1\AppData\Local\Temp\pip-build-env-ajqw1i5n\overlay\site-packages\setuptools\command\build_ext.py", line 247, in build_extension if ext._needs_stub: AttributeError: 'Extension' object has no attribute '_needs_stub' ```

maybe because finalize_options needs to take the results of configure into account? I don't really understand why only one case would be affected, but I think some options may need to take configure results into account for configure to properly have an effect.

mgorny commented 1 year ago

Do you have any idea how to resolve that? I don't think distutils were designed with running commands from finalize_options() in mind.

minrk commented 1 year ago

Not off the top of my head. Can you share your Python, OS, and setuptools versions? Because when I try this (mac, Python 3.10.9, setuptools 67.8.0), it successfully builds with -j12 and no errors.

setuptools finalize_options modifies Extension objects, which means that all Extension objects must be defined before finalize_options is called (arguably it shouldn't do that, since it's not related to options). configure may add an Extension for the bundled pyzmq, so it needs to run before build_ext.finalize_options.

If finalize_options is idempotent, we could perhaps run dist.get_command_object("build_ext").finalize_options() . I'm not sure that's a guarantee, though.

Ideally, 'configure' would run entirely before creating the distribution so there wouldn't be any of these interactions, but it needs the distribution to run. I'm not sure if setup can be invoked twice, so that we can effectively do python setup.py configure && python setup.py build_ext in one setup.py script?

mgorny commented 1 year ago

Not off the top of my head. Can you share your Python, OS, and setuptools versions? Because when I try this (mac, Python 3.10.9, setuptools 67.8.0), it successfully builds with -j12 and no errors.

Ah, I'm sorry. I have assumed this happens for everyone but apparently I was wrong.

OS: Gentoo Linux amd64 Python: 3.10.11, 3.11.3 setuptools: 67.8.0

However, from a quick experimentation it doesn't happen in a "pure" venv, so I suspect one of the installed packages is causing this. I'm going to investigate deeper.

Ideally, 'configure' would run entirely before creating the distribution so there wouldn't be any of these interactions, but it needs the distribution to run. I'm not sure if setup can be invoked twice, so that we can effectively do python setup.py configure && python setup.py build_ext in one setup.py script?

Well, this is a really hard problem. Note that per PEP517, you shouldn't be expecting any setup.py calls at all. The only reason we're calling build_ext directly is that setuptools currently doesn't support passing -j and having it passed speeds builds a lot.

Now that I have a hint where to look next, I'm going to try and see if this isn't a bug somewhere else to fix.

minrk commented 1 year ago

Thanks! Let me know what you find. I'm also fully open to calling self.parallel = int(self.parallel) if this is mostly a one-off issue.

per PEP517, you shouldn't be expecting any setup.py calls at all

That's not quite true. This repo follows PEP517 to explicitly use the setuptools build backend, which in turn executes setup.py.

I've been curious about something like hatchling to possibly run the compilation in a different way, but I'm reluctant because setup.py now handles so many edge cases that I'm sure I would introduce regressions if I did that. It's not currently using any deprecated APIs, though.

mgorny commented 1 year ago

Well, it seems to be caused by numpy overriding the C compiler somehow:

Traceback (most recent call last):
  File "/tmp/pyzmq/setup.py", line 1367, in <module>
    setup(**setup_args)
  File "/usr/lib/python3.10/site-packages/setuptools/__init__.py", line 107, in setup
    return distutils.core.setup(**attrs)
  File "/usr/lib/python3.10/site-packages/setuptools/_distutils/core.py", line 185, in setup
    return run_commands(dist)
  File "/usr/lib/python3.10/site-packages/setuptools/_distutils/core.py", line 201, in run_commands
    dist.run_commands()
  File "/usr/lib/python3.10/site-packages/setuptools/_distutils/dist.py", line 969, in run_commands
    self.run_command(cmd)
  File "/usr/lib/python3.10/site-packages/setuptools/dist.py", line 1244, in run_command
    super().run_command(command)
  File "/usr/lib/python3.10/site-packages/setuptools/_distutils/dist.py", line 987, in run_command
    cmd_obj.ensure_finalized()
  File "/usr/lib/python3.10/site-packages/setuptools/_distutils/cmd.py", line 111, in ensure_finalized
    self.finalize_options()
  File "/tmp/pyzmq/setup.py", line 1259, in finalize_options
    self.distribution.run_command("configure")
  File "/usr/lib/python3.10/site-packages/setuptools/dist.py", line 1244, in run_command
    super().run_command(command)
  File "/usr/lib/python3.10/site-packages/setuptools/_distutils/dist.py", line 988, in run_command
    cmd_obj.run()
  File "/tmp/pyzmq/setup.py", line 765, in run
    self.check_zmq_version()
  File "/tmp/pyzmq/setup.py", line 469, in check_zmq_version
    detected = self.test_build(zmq_prefix, self.compiler_settings)
  File "/tmp/pyzmq/setup.py", line 717, in test_build
    detected = detect_zmq(self.tempdir, compiler=self.compiler, **settings)
  File "/tmp/pyzmq/buildutils/detect.py", line 114, in detect_zmq
    if not compiler.has_function('timer_create'):
  File "/usr/lib/python3.10/site-packages/setuptools/_distutils/ccompiler.py", line 893, in has_function
    objects = self.compile([fname], include_dirs=include_dirs)
  File "/usr/lib/python3.10/site-packages/numpy/distutils/ccompiler.py", line 89, in <lambda>
    m = lambda self, *args, **kw: func(self, *args, **kw)
  File "/usr/lib/python3.10/site-packages/numpy/distutils/ccompiler.py", line 278, in CCompiler_compile
    _job_semaphore = threading.Semaphore(jobs)
  File "/usr/lib/python3.10/threading.py", line 423, in __init__
    if value < 0:
TypeError: '<' not supported between instances of 'str' and 'int'

To reproduce it, you have to:

pip install cython numpy pythran
python setup.py build_ext -j12
minrk commented 1 year ago

Weird, numpy modifies distutils just by being installed? WDYT of special-casing self.parallel as a simpler one-off fix without having to worry about the bigger, general issue of finalize_options?

FWIW, if the installer follows PEP 517 (e.g. installation with pip and without --no-build-isolation, the standard way ~every Python package is installed), this can't happen because numpy won't be in the build environment.

mgorny commented 1 year ago

Weird, numpy modifies distutils just by being installed? WDYT of special-casing self.parallel as a simpler one-off fix without having to worry about the bigger, general issue of finalize_options?

Actually, it happens because Cython imports Pythran if the latter is installed, and Pythran imports numpy.distutils that causes the overrides to happen.

I'll update the PR to copy the self.parallel logic from setuptools/distutils.

FWIW, if the installer follows PEP 517 (e.g. installation with pip and without --no-build-isolation, the standard way ~every Python package is installed), this can't happen because numpy won't be in the build environment.

Yeah, I suppose isolated builds are reasonably secure against that.

mgorny commented 1 year ago

I've updated the PR and hopefully added clear enough comment as why this is done.

mgorny commented 1 year ago

Thanks for the update. Do you need me to do anything more?

minrk commented 1 year ago

Thanks!

mgorny commented 1 year ago

Thanks a lot.

We have worked around it locally already, so there's no need for a new release because of this. We can wait for the next regular release.