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

Remove astropy-helpers #1047

Merged
merged 16 commits into from
Sep 25, 2022
Merged

Remove astropy-helpers #1047

merged 16 commits into from
Sep 25, 2022

Conversation

aphearin
Copy link
Contributor

This PR follows the instructions provided in APE 17 to remove the halotools dependency on astropy-helpers.

@aphearin aphearin self-assigned this Sep 19, 2022
@aphearin
Copy link
Contributor Author

Thanks to @astrofrog and @Cadair for putting together the very clear instructions for how to replace the astropy-helpers package with standard tooling. I have done my best to follow the APE 17 instructions, including using cookiecutter to generate templates for setup.py, pyproject.toml, tox.ini, etc. I think I am very close, but I am hung up on something that I'm having trouble finding. I get the same traceback (copied below) for:

  1. my CI tests run via a github action
  2. my docs build done on RTD via a webhook
  3. local tests using tox -e test on my machine
  4. local attempts to build the distribution using python -m build --sdist .

All of the above produce basically the same traceback, so I am hopeful that once the cause of the error is identified, that halotools will become independent of the astropy-helpers and ready to release v0.8. Any help you can provide in identifying this issue would be much appreciated.

$ tox -e test
.package create: /Users/aphearin/work/repositories/python/halotools/.tox/.package
.package installdeps: setuptools, setuptools_scm, wheel, cython==0.29.32, oldest-supported-numpy, extension-helpers
Traceback (most recent call last):
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/pkg_resources/_vendor/packaging/requirements.py", line 102, in __init__
    req = REQUIREMENT.parseString(requirement_string)
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/pkg_resources/_vendor/pyparsing/core.py", line 1141, in parse_string
    raise exc.with_traceback(None)
pkg_resources._vendor.pyparsing.exceptions.ParseException: Expected string_end, found 'numpy'  (at char 8), (line:1, col:9)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/aphearin/opt/miniconda3/envs/dstar/lib/python3.9/site-packages/tox/helper/build_requires.py", line 16, in <module>
    for_build_requires = backend.get_requires_for_build_sdist(None)
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/build_meta.py", line 341, in get_requires_for_build_sdist
    return self._get_build_requires(config_settings, requirements=[])
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/build_meta.py", line 320, in _get_build_requires
    self.run_setup()
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/build_meta.py", line 335, in run_setup
    exec(code, locals())
  File "<string>", line 79, in <module>
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/__init__.py", line 86, in setup
    _install_setup_requires(attrs)
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/__init__.py", line 78, in _install_setup_requires
    dist.parse_config_files(ignore_option_errors=True)
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/_virtualenv.py", line 21, in parse_config_files
    result = old_parse_config_files(self, *args, **kwargs)
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/dist.py", line 870, in parse_config_files
    self._finalize_requires()
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/dist.py", line 577, in _finalize_requires
    self._move_install_requirements_markers()
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/setuptools/dist.py", line 617, in _move_install_requirements_markers
    inst_reqs = list(_reqs.parse(spec_inst_reqs))
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/pkg_resources/__init__.py", line 3102, in __init__
    super(Requirement, self).__init__(requirement_string)
  File "/Users/aphearin/work/repositories/python/halotools/.tox/.package/lib/python3.9/site-packages/pkg_resources/_vendor/packaging/requirements.py", line 104, in __init__
    raise InvalidRequirement(
pkg_resources.extern.packaging.requirements.InvalidRequirement: Parse error at "'numpy sc'": Expected string_end
ERROR: invocation failed (exit code 1), logfile: /Users/aphearin/work/repositories/python/halotools/.tox/.package/log/.package-2.log
================================================================================================ log start =================================================================================================

================================================================================================= log end ==================================================================================================
ERROR: FAIL could not package project - v = InvocationError("/Users/aphearin/work/repositories/python/halotools/.tox/.package/bin/python /Users/aphearin/opt/miniconda3/envs/dstar/lib/python3.9/site-packages/tox/helper/build_requires.py setuptools.build_meta '' ''", 1)

setup.cfg Outdated Show resolved Hide resolved
@aphearin
Copy link
Contributor Author

In all my builds, I now get an error message related to compiling cython:

clang: error: '-I-' not supported, please use -iquote instead

This PR does not touch any of the cython, and so I think maybe the migration of the tooling has inadvertently changed the compiler in some way. @Cadair does this seem like the right explanation? I'm not finding a lot of useful information online about this error. A lot of the cython uses the -Ofast flag in extra_compile_args, but removing all appearances of this flag does not resolve the issue (or even change the traceback).

Copying more of the traceback in case it helps:

gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/ -It -Im -Ip -I/ -Ip -Ii -Ip -I- -Ib -Iu -Ii -Il -Id -I- -Ie -In -Iv -I- -Iz -Ix -Ij -If -Id -I5 -Iv -I4 -I/ -Io -Iv -Ie -Ir -Il -Ia -Iy -I/ -Il -Ii -Ib -I/ -Ip -Iy -It -Ih -Io -In -I3 -I. -I9 -I/ -Is -Ii -It -Ie -I- -Ip -Ia -Ic -Ik -Ia -Ig -Ie -Is -I/ -In -Iu -Im -Ip -Iy -I/ -Ic -Io -Ir -Ie -I/ -Ii -In -Ic -Il -Iu -Id -Ie -I/home/runner/work/halotools/halotools/.tox/py39-test-alldeps/include -I/opt/hostedtoolcache/Python/3.9.13/x64/include/python3.9 -c halotools/empirical_models/occupation_models/engines/cacciato09_sats_mc_prim_galprop_engine.cpp -o build/temp.linux-x86_64-cpython-39/halotools/empirical_models/occupation_models/engines/cacciato09_sats_mc_prim_galprop_engine.o -Ofast
      cc1plus: note: obsolete option ‘-I-’ used, please use ‘-iquote’ instead
      cc1plus: error: ‘-I-’ specified twice
      cc1plus: note: obsolete option ‘-I-’ used, please use ‘-iquote’ instead
      cc1plus: error: ‘-I-’ specified twice
      cc1plus: note: obsolete option ‘-I-’ used, please use ‘-iquote’ instead
      cc1plus: error: ‘-I-’ specified twice
      cc1plus: note: obsolete option ‘-I-’ used, please use ‘-iquote’ instead
      error: command '/usr/bin/gcc' failed with exit code 1
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for halotools
Failed to build halotools

@aphearin
Copy link
Contributor Author

Ok so one set of explanations here is that by updating to macos-11 that maybe my pretty old cython source code is just no longer compatible with up-to-date versions of clang. In which case, the resolution would require updating the cython source code to be compatible with the updated compilers.

To investigate that, I started poking around for info about clang updates in the macos-11 series, but then I remembered that the current source code had no trouble very recently with creating a build wheel when doing so with this github action. For example, there is a successful wheel generated here. Based on this, I think the issue must somehow lie with either the config specified by the current PR in pyproject.toml and setup.cfg, and/or the use of the extension-helpers package, since those are the two main new things on the cython side that have changed over the last two days since buildling that wheel.

@aphearin
Copy link
Contributor Author

aphearin commented Sep 23, 2022

I have been exploring solutions to this problem using two minimal reproducer repositories. The first repo here is based on setup.py, and the second repo here was configured using cookiecutter and is based on pyproject.toml.

As pointed out here, the root issue seems to be related to the basic mechanism for creating multiple cython extensions using the get_extensions() mechanism implemented in the extensions-helpers package. Then, as pointed out here, I found a simple fix in which the np.get_include() is replaced by ['numpy'] within the get_extensions() function. In the case of the minimal repository, this actually worked, and the code built just fine. However, this is not what is recommended in APE 17. And moreover, implementing this within halotools here leads to a separate error:

clang -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -fwrapv -O2 -Wall -fPIC -O2 -isystem /Users/aphearin/opt/miniconda3/envs/dstar/include -fPIC -O2 -isystem /Users/aphearin/opt/miniconda3/envs/dstar/include -Inumpy -I/Users/aphearin/work/repositories/python/halotools/.tox/test/include -I/Users/aphearin/opt/miniconda3/envs/dstar/include/python3.9 -c halotools/empirical_models/occupation_models/engines/cacciato09_sats_mc_prim_galprop_engine.cpp -o build/temp.macosx-10.9-x86_64-cpython-39/halotools/empirical_models/occupation_models/engines/cacciato09_sats_mc_prim_galprop_engine.o -Ofast
      halotools/empirical_models/occupation_models/engines/cacciato09_sats_mc_prim_galprop_engine.cpp:760:10: fatal error: 'numpy/arrayobject.h' file not found
      #include "numpy/arrayobject.h"
               ^~~~~~~~~~~~~~~~~~~~~
      1 error generated.
      error: command '/usr/bin/clang' failed with exit code 1
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for halotools
Failed to build halotools

So I think the workaround to replace np.get_include() --> ['numpy'] may not actually be the right solution, even though it happens to work for the case of the two simple repositories. Suggestions from @astrofrog @Cadair or anyone with relevant expertise from the extensions-helpers package otherwise would be very much appreciated.

@aphearin
Copy link
Contributor Author

Ok so by the end of yesterday, I made some headway on a different approach in which I revert to using an elementary setup.py package layout for halotools, drop the extensions-helpers dependency, and manually adding the cython extensions by hand. I started doing this module by module (using np.get_include()) and I got far enough along that I convinced myself that this is going to work. Probably the best thing at this point is for me to finish building the package that way, and then go ahead and release v0.8 with that layout. Then I can just ping the folks at extensions-helpers in case they want to try and support this use case by comparing what works vs. what doesn't.

@astrofrog
Copy link
Member

@aphearin - I'd like to help try and get this working with extension-helpers - should I try and see if I can get it to work ignoring the last commit here or is there a better place to start?

@astrofrog
Copy link
Member

See aphearin/cython_bug_demo#3

@aphearin
Copy link
Contributor Author

Oh, great catch @astrofrog! This makes the compiler errors make much more sense - they seemed like some weird string was being passed as a compiler argument, and so I bet your fix to the minimal reproducer will also work with halotools. The most recent commit 2e1e2bb now has passing tests on CI after abandoning the extensions-helpers, but I can roll that back and continue to use the extensions-helpers if you foresee continued support for this library. Either way, I know it's an awful lot of source code and extensions, so many thanks for taking a look and (hopefully) finding the fix!

@astrofrog
Copy link
Member

We will definitely be supporting extension-helpers going forward so I would recommend continuing to use it if you are happy to!

@aphearin
Copy link
Contributor Author

Ok, great, yes I'm happy to keep the extensions-helpers dependency then. I'll go ahead and implement your fix.

@aphearin
Copy link
Contributor Author

Thanks again for the help @astrofrog. Your fix was all that was needed in order to get halotools working again without astropy-helpers and using pyproject.toml. I'm very happy to have this working again since the main purpose of the v0.8 release is to update the infrastructure of the library and bring halotools up-to-date with its dependency chain and modern package tooling. Much appreciated!

@aphearin aphearin merged commit 607fcbf into astropy:master Sep 25, 2022
@aphearin aphearin deleted the remove_helpers branch September 25, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants