Build updates (move towards PEP-517 support) #1329
Conversation
|
@Marie-Terrell @harlantc - here's the build rework I recently mentioned. I've left this as a draft as it needs #1328 to land first and there's likely more cleanup to do (including rewording the current WIP commit....) |
DougBurke
force-pushed
the
pep-517-changes
branch
8 times, most recently
from
579eaf3 to
855b730
Compare
DougBurke
force-pushed
the
pep-517-changes
branch
from
e0f2501 to
5f0d803
Compare
DougBurke
force-pushed
the
pep-517-changes
branch
3 times, most recently
from
73b77ee to
054f75d
Compare
DougBurke
force-pushed
the
pep-517-changes
branch
2 times, most recently
from
0d710ed to
6a84504
Compare
|
Just so that I don't forget when talking about build systems: Scipy has decided that setuptools is not enough for them and they are moving to meson: https://labs.quansight.org/blog/2021/07/moving-scipy-to-meson/ |
|
that page seems to be MIA now, @hamogu , but I found Scipy's git discussion on it. scipy/scipy#13615 |
|
There is also this [old] post about using meson / other systems (which ends up linking to the scipy discussion) at https://discuss.python.org/t/should-python-packaging-aim-for-meson-as-build-system-in-case-of-extension-modules/2579 |
DougBurke
force-pushed
the
pep-517-changes
branch
from
6a84504 to
be2d4be
Compare
DougBurke
force-pushed
the
pep-517-changes
branch
2 times, most recently
from
6108dfe to
72a9fa9
Compare
Codecov Report
@@ Coverage Diff @@
## main #1329 +/- ##
===========================================
- Coverage 78.95% 66.32% -12.64%
===========================================
Files 72 72
Lines 26673 26676 +3
Branches 4496 4496
===========================================
- Hits 21060 17692 -3368
- Misses 5420 8877 +3457
+ Partials 193 107 -86
Continue to review full report at Codecov.
|
DougBurke
force-pushed
the
pep-517-changes
branch
from
72a9fa9 to
97ddfd7
Compare
Remove a number of warning checks (for un-installed modules) since it's assumed that this is now handled by setup.cfg. We can still 'python setup.py develop' but do we care about this?
The setuptools-specific commands are being removed [1] so remove the clean command as 1. we can use 'git clean -dfx .' to remove extra files 2. it's not at all obvious that setuptools will still support this command (it was from distutils but this code has been moved into setuptools and is being "simplified") [1] https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html
numpy.distutils is now deprecated, so replace the easy-to-identify
changes. One way I have used to check is to use the command
python -m build
to build the code following PEP 517.
Several cases where we currently can't replace the numpy.distutils
version are noted.
It appears that options.entry_points in setup.cfg was added in version 51.0.0 of setuptools, so bump the minimum version. Limited testing didn't show a problem if I forced a setuptools earlier than 51.0.0, but https://setuptools.pypa.io/en/latest/userguide/declarative_config.html#options lists 51.0.0 as the version options.entry_points was added
The setuptools document at https://setuptools.pypa.io/en/latest/userguide/declarative_config.html#options points out that options.data_files is deprecated, and we should do something else, but - as with everything python-packaging related - when I go to the "read here how to do it" page I do not understand what they are saying. It looks like I can just use options.package_data to refer to the sherpa[-standalone].rc files, but it triggers build issues. This appears to be because a) helpers/sherpa_config.py implicitly requires on the data_files option (one issue in doing this is understanding how a configuration option maps to various fields in the setuptools Command object, how thy are meant to be used, and how we are using them) b) the reason we use the data_files field is to include the stack and group "modules" if needed (primarily if in a non-CIAO build), and this is something we've wanted to address for a long time: sherpa#50
Instead of explicitly mentioning the parent class, use super(). There is also a small change in the develop routine so that a) we announce the stk and group extensions separately b) we only announce them when they are actually built However it looks like we don't see these messages by default, so most users will not see any difference.
I had noted that sdist used numpy.distutils.misc_util.get_data_files and what should be the replacement for this. In testing, I've found that I can't trigger the add_defaults method which calls this routine, so I have replaced it with a method that errors out if called (in case my testing, which has been limited, was not sufficient).
Numpy warns if you use the setuptools sdist - see numpy/numpy#7131 - so revert this change for now.
Prior to this commit the XSPEC relevant code and tests were dropped when XSPEC support was disabled. This made sense, but there have been times when the installation could still install the XSPEC tests, with the result that 'import sherpa.astro.xspec' would not fail even though XSPEC support was not enabled. This behavior seems to have been fixed with the PEP 517 changes, but it is not-at-all-clear why it happened, so it seems better to guard against this feature of the build infrastructure. See sherpa#1545 for more information. Instead, we now ensure the XSPEC code is installed, even when XSPEC support is disabled. This can lead to an unsettling error message from 'import sherpa.astro.xspec', so the XSPEC initialization code has been tweaked to provide a nicer message in this case: % python -c 'import sherpa.astro.xspec' Traceback (most recent call last): File "/home/dburke/miniconda3/envs/sherpa-pr/lib/python3.9/site-packages/sherpa/astro/xspec/utils.py", line 24, in <module> from . import _xspec ImportError: cannot import name '_xspec' from partially initialized module 'sherpa.astro.xspec' (most likely due to a circular import) (/home/dburke/miniconda3/envs/sherpa-pr/lib/python3.9/site-packages/sherpa/astro/xspec/__init__.py) The above exception was the direct cause of the following exception: Traceback (most recent call last): File "<string>", line 1, in <module> File "/home/dburke/miniconda3/envs/sherpa-pr/lib/python3.9/site-packages/sherpa/astro/xspec/__init__.py", line 113, in <module> from .utils import ModelMeta, version_at_least, equal_or_greater_than File "/home/dburke/miniconda3/envs/sherpa-pr/lib/python3.9/site-packages/sherpa/astro/xspec/utils.py", line 31, in <module> raise ImportError("XSPEC support is not enabled") from ie ImportError: XSPEC support is not enabled Note that we retain the original "hard to understand for most users" error, but only as a "secondary" one - the main error tells the user the problem. We keep the original error in case there is a problem in XSPEC initialization, which we'd like to know about.
The --verbose flag is included as it can be useful to see how the build was done when there's a problem.
I don't know why the _xpsec.cc file needs to be included but the other files do not.
Looking cosely at the message from NumPy, it looks like we have to use distutils after all. In doing so, we trigger the add_defaults behavior. In checking out the old method, it was only being used to add in the stack and group dynamic libraries, and this doesn't actually work (they get rejected). So I removed the add_defaults method. If sherpa#50 gets fixed up we may have to do something, but I don't think so because they are both compiled code and the sdist is not for those files (at least as far as I know).
As requested by @Marie-Terrell, I have restored the check on the Python version just in case people with old systems try to build Sherpa. As mentioned in the comments, I a) would prefer to rely on the metadata in setup.cfg to catch this, but if people are using old tool chains they will not pick up on this b) I would hope this is now an uncommon situation so it's not worth time on updating the error message (i.e. once we drop Python 3.7 support).
There's no functional change here, but the code uses more-modern Python idioms (the decision of when to stop is somewhat arbitrary, I basically got to the point of understanding what is going on and then stopping).
DougBurke
force-pushed
the
pep-517-changes
branch
from
8f509d7 to
a31d331
Compare
|
I've re-based as I wasn't sure whether we had any "build" changes needed in main. I've put back the version check that @Marie-Terrell was asking for (I am personally less bothered about this as python 3.6 is now "old" and I wanted to remove as much cruft as possible but I don't see this as requirement). I also added a minor clean-up of |
|
For @Marie-Terrell - as I mention in the details section of the PR, the XSPEC change is because of
What I need to know if this is good, bad, or indifferent. |
There was a problem hiding this comment.
For the rest of my review, see my comment from July 18th.
The xspec change isn't ideal, but I think since this catches a case with issues while still throwing a more explanatory error than what was seen before #1458 it is a great compromise (which makes it the best option currently).
Ran through a couple quick checks of the helpers/deps/init.py change just to confirm that nothing unexpected changed.
This looks good. Thank you!
Co-authored-by: wmclaugh <[email protected]>
| ] | ||
| make("install", opts, check=True) | ||
|
|
||
| # Create the built file to indicate the dependecies have been |
There was a problem hiding this comment.
| # Create the built file to indicate the dependecies have been | |
| # Create the built file to indicate the dependencies have been |
There was a problem hiding this comment.
@wmclaugh - let me know when you have finished so I don't accidentally add your fixes before you've finished. As that may have just happened here already....
Summary
Move the Sherpa build system towards a more static configuration (PEP 517)
Details
October 22 2021 saw the release of setuptools 58.3.0 where the
installmethod was marked as deprecated - https://setuptools.pypa.io/en/latest/history.html#v58-3-0 - so there's "more urgency" to this. Added April 14 2022 I've been involved in a discussion on how to build C extensions thread at https://discuss.python.org/t/modern-way-to-build-packages-with-c-cython-extensions/15050 which - at the time of writing - suggests that staying withpython setup.py developis reasonable for us (from one respondent).This work was based on @hamogu's heroic #784 (but doesn't get to some of the good bits from that work, such as supporting tox or switching to
setuptools_scm; they can come later). It has seen numerous rebases and reworks as I've tried to get parts of this work merged.The main changes are:
perform some code cleanup and changes to address newer standards and build systems
helpers/has been re-arranged so thathelpers.extensions.build_extis a bit easier to use (it combines the "grab the settings" and the "build the actual extension" steps) and this happened because I realized we were including the wcs libs/headers when building the region code which is not needed (at least in the non-CIAO build)bump to a minimum version of 49.1.2 for setuptools as it is in this release that they include the
distutilscode (which is technically needed to build nicely with >= python 3.10 but it's not obvious if it's required). As part of this we now extend thenumpy.distutils.command.sdistcommand rather thandistutils.command.sdist- this "feels" right but it could have unintended consequences and I can't know remember whether it was needed or not (it's been really hard to track down what any one change does to the build system...)there are then two MANIFEST changes - the first one should be uncontroversial but the second one is a bit unclear to me as I am currently reading about how unclear all this is:
the next commit is
Move basic options to setup.cfgwhich moves over much of the information fromsetup.pytosetup.cfg, and removes some of the repetition with the use of thepackages = find:option. Note that we'll come back to that in a commit or two...aha:
setup.cfg: explicit about packages and testsmeans I go back to being explicit about the packages and tests. This technically should not be needed but I was seeing some odd behavior and being explicit is "not necessarily bad" and potentially helps out with things like the settings queried by xspec_config.back to code removal:
Simplify setup.pyremoves some code that was to support building when you might not have setuptools - but we now require it. I also ensuresetuptoolsis imported insetup.pyas I was seeing "odd" issues with this - this may not be necessary with setuptools >=49.1.2 (or perhaps later) but I do know that setuptools does "fun" stuff with imports related to distutils. At this point I think you can still say 'python setup.py develop` but I don't want to bother worrying about that.in an earlier commit I changed the
cleancommand to allow it to work with setuptools >= 49.1.2, but we know that setuptools wants to remove such commands so inRemove the clean command from setupI have removed the clean command. I have never used thecleancommand, as I findgit clean -dfx .to be "good enough" (and probably better than the currentcleancommand). For referencethere are some more replacements of distutils functionality - it's a bit hit or miss on what seems to work, but that's in part because we are trying to use an old setuptools because of some work done in Improve several issues when building and testing Sherpa #1458
looking at the setuptools documentation - in particular https://setuptools.pypa.io/en/latest/userguide/declarative_config.html#options - I noted that the
entry_pointsoption was listed as added in version 51, so we now require this as a minimum version rather than 49.1.2. Now, it's not obvious to me that this is needed, since I could not obviously trigger problems with an older setuptools version, but I'm not sure it really matters (as version 51.0.0 of setuptools was released December 2020).In looking at https://setuptools.pypa.io/en/latest/userguide/declarative_config.html#options I noted that the
data_filesoption was marked as deprecated, so I then tried to address this. Unfortunately a combination of the documentation they point to https://setuptools.pypa.io/en/latest/userguide/datafiles.html and our own code made this a surprisingly-involved exploration, even though the actual code change is small. I'm relying on the fact that things still build and the tests still run to validate this change. As I note below, this leads to looking at issue group.so and stk.so are installed outside of the sherpa namespace #50, as this is one place we care about data files and locations and the like, but I think that can be looked at separately from this PR.there is then a change to basically use
superrather than the explicit class name. This is mainly a code-style change but there is technically a change in behavior in the announcing of building the stack and group libraries during a develop build - we now only report things when they are actually changed, and separate versions rather than a combined version - but I think these messages aren't seen by default anyway.sdist change: I noted earlier that I did not know how to replace the numpy.distutils command
get_data_filesbut my claim is that we don't need to, as I could not trigger this bit of code to file. I've replaced the method with something that falls over (rather than removing the method) so we will find out if we do actually use it. However, our CI run doesn't trigger it, and manual calls topython setup.py sdistdoesn't seem to either.sdist: revert the setuptools change. If you ran
python setup.py sdistthen you got a warning from numpy that the setuptoolssdistcommand could cause problems - and links to Fix setuptools sdist numpy/numpy#7131 (found by @Marie-Terrell ). So I have reverted this change. However, see theNotes - sdistsection below.in an attempt to fix installing sherpa; interesting version differences #1545 I have re-implemented the change from do not change the XSPEC configuration when Sherpa is being built #1506 to always include the XSPEC code and tests, even when XSPEC is not being built. The assumption has been that 'import sherpa.astro.xspec' would error out cleanly if XSPEC support was not present, but I was able, in installing sherpa; interesting version differences #1545, get to a place where this was not true. I have no idea what causes this condition to happen, and it seems to depend on various versions of python/setuptools/..., but the fact that it can happen makes me worry about it. Now, with this change and an XSPEC-free build a call to 'import sherpa.astro.xspec' would trigger a confusing recursive-import message from
._xspec, so we now catch this and add a relatedXSPEC support is not enabledimport error to be more-understandable (but keep the original error which is needed to debug potential XSPEC library issues for when we do have XSPEC support enabled).finally I switch the GH workflow calls to build with pip rather than directly call setup.py
pip --verboseflag to get the build output included in the log. This is not technically needed, and makes going through the logs slower (more to download) but does give us access to information on how the build works, which I think is worth it.Notes
So, in the move to "declarative metadata"
setuptools 30.3.0(Dec 2016) stated to usesetup.cfg- see https://setuptools.pypa.io/en/latest/userguide/declarative_config.htmlsetuptools 61.0.0(March 2022) now usespyproject.toml(butsetup.cfgis still supported) since this is the now-agreed-upon place - see https://setuptools.pypa.io/en/latest/userguide/pyproject_config.htmland we are sticking with
pyproject < 60to follow the NumPy advice as they update their build system - https://numpy.org/doc/stable/reference/distutils_status_migration.html . I think we don't require anything "fancy", so should be able to move away from usingnumpy.distutils, but I still haven't seen any "to build an extension that uses the NumPy C API you just need to do this sparkly thing" information out there.The release of NumPy 1.23 now causes this warning to be displayed when you build Sherpa:
Some of the changes, such as the
data_fileschange, are related to issue #50 - it would be nice to address this, but it doesn't have to be in this PR.Notes - sdist
Even with going back to the
numpy sdistI still see a warning about setuptools, so I just give up (maybe it's saying we should use distutils not numpy.distutils here?)--
References
Configuration
We can see that
setup.cfgnow contains most of the configuration options that were insetup.py(the remaining fields areversion,ext_modules, andcmdclass):Things you can now do
PEP 517 builds
The following seem to work, but have seem limited testing (also, the builds used to fail but some changes we've done now mean that even with the
mainbranch you can build a wheel; it's just that when you come to test the result themainbranch may fail):Build using a PEP 517 tool - e.g. build
I tried changing
setup.cfgto enable XSPEC support and usedpython -m build .; pip install dist/.....whland it built and installed an XSPEC-capable Sherpa (at least once I fixed theMANIFEST.infile).You can also try
and then the following should work
how to test
This is now the suggested way to test an editable build (ie not an installed one):
You can test an installed version (which is different to
python setup.py test) by sayingalthough we also have the
sherpa_testcommand that essentially does this for us.