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

Replace distutils with setuptools #1050

Merged
merged 2 commits into from
Jul 12, 2022

Conversation

brandonwillard
Copy link
Member

@brandonwillard brandonwillard commented Jul 12, 2022

This PR replaces distutils imports with the equivalent setuptools imports, in anticipation of distutils's deprecation.

The numpy.distutils changes in this PR may affect the "dynamic" determination of build settings, since the old numpy.distutils.system_info.get_info appears to rely on the discovered pkg-config binary in one step, and that binary could be arbitrarily changed. The approach used in this PR—i.e. numpy.__config__.get_info—relies on the system_info settings at the time when numpy was built, and appears to return the appropriate environment-specific library/include paths.

Since the underlying parameters are all still configurable via aesara.config and/or environment variables, I'm not too concerned about losing any fringe functionality with this change, especially not when the parameters we're trying to obtain are explicitly the parameters used by NumPy (i.e. the ones given by numpy.__config__.get_info), and not the ones given by an arbitrary pkg-config in one's path or the like. My guess is that the latter actually leads to more errors and confusion than benefits.

@brandonwillard brandonwillard added enhancement New feature or request important labels Jul 12, 2022
@brandonwillard brandonwillard self-assigned this Jul 12, 2022
@brandonwillard brandonwillard force-pushed the remove-distutils branch 2 times, most recently from 05a2c16 to 8b96379 Compare July 12, 2022 17:02
@brandonwillard brandonwillard linked an issue Jul 12, 2022 that may be closed by this pull request
@brandonwillard
Copy link
Member Author

I wonder if these changes help with #1005, #980, and conda-forge/aesara-feedstock#54.

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.25%. Comparing base (d69eaab) to head (1f31a97).
Report is 505 commits behind head on main.

Files Patch % Lines
aesara/link/c/cmodule.py 78.57% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
- Coverage   79.25%   79.25%   -0.01%     
==========================================
  Files         152      152              
  Lines       47887    47882       -5     
  Branches    10909    10909              
==========================================
- Hits        37955    37949       -6     
- Misses       7435     7436       +1     
  Partials     2497     2497              
Files Coverage Δ
aesara/configdefaults.py 66.20% <100.00%> (ø)
aesara/link/c/exceptions.py 80.00% <100.00%> (ø)
aesara/tensor/blas.py 79.71% <100.00%> (-0.02%) ⬇️
aesara/link/c/cmodule.py 54.34% <78.57%> (-0.23%) ⬇️

@brandonwillard
Copy link
Member Author

brandonwillard commented Jul 12, 2022

@aesara-devs/core, I'm ready to merge this, but I'm a little unsure about the setuptools dependency added to setup.py. Since we use setuptools at run-time now, it seems appropriate, but it's also understood to be present as a requirement for the Aesara installation itself, and I'm not sure how the version dependency aspects of that should work.

@brandonwillard brandonwillard merged commit be222f0 into aesara-devs:main Jul 12, 2022
@brandonwillard brandonwillard deleted the remove-distutils branch July 12, 2022 20:33
@maresb
Copy link
Contributor

maresb commented Jul 13, 2022

I don't use setuptools very much, but you might consider adding a pyproject.toml and seeing to what extent you can replace setup.py (imperative) and setup.cfg (declarative) with that.

For the Aesara installation itself, the pyproject.toml would define your build system to be setuptools and define any additional build-time requirements (plugins) there. Correspondingly on the Conda side, those are put into the host requirements. So based on the above, we will need to add setuptools as both a host and run requirement in the recipe.

I see that you very rarely change the dependencies, so it generally makes sense to have bot-automerge is enabled on the feedstock. However with the current change, this will cause the autotick-bot PR to merge on passing CI, but a passing CI does not indicate that the recipe is correct. (For instance, there may be a missing direct dependency which is coincidentally added indirectly.)

I recommend disabling automerge, at least for now, so that we can make sure the dependencies are correctly declared. You can always add the automerge label to an individual PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace usage of numpy.distutils
2 participants