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

TST: Test that packages can be build from sdists. #532

Merged
merged 28 commits into from
Feb 5, 2022

Conversation

DWesl
Copy link
Contributor

@DWesl DWesl commented Jan 31, 2022

I ran into problems trying to install basemap on a somewhat unusual platform (Cygwin, but the relevant bit is that it doesn't have binary wheels).

@molinav
Copy link
Member

molinav commented Jan 31, 2022

Hi @DWesl!

Thank you very much for the contribution. You are totally right, I was quite focused on creating working wheels and I didn't realise that the sdist zip is missing the requirements files that later setup.py reads.

@DWesl
Copy link
Contributor Author

DWesl commented Jan 31, 2022

I appear to have broken the test workflow, though similar commands work fine on my laptop. Should I drop the attempt to test the source distribution and just include the changes to MANIFEST.in and/or pyproject.toml?

@molinav
Copy link
Member

molinav commented Jan 31, 2022

Most of the failures are coming because the precompiled GEOS library is not being found, and then all references to its symbols are identified as undeclared.

I will need to take a look on it, probably during this evening.

@molinav
Copy link
Member

molinav commented Feb 4, 2022

Sorry for coming late to the PR, I was a bit busy this week. For the moment I reverted the changes in the workflows, since they break the pipelines. I am not fully aware on how python -m build works, but it seems it cannot find the precompiled GEOS and numpy, and that is why the pipelines fail (maybe because it builds in a separate virtual environment and the working directory is changed?).

The most important part is your addition of the MANIFEST.in file; without that, the source distribution was broken. I also included some additional files in the manifest.

Something that would be easy to implement in the GitHub Actions is to also try to install the packages from the source distribution. At the moment it only tries to install the wheels. If I had added this in the test job, this error would have appeared before.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 4, 2022

python -m build will create an sdist, then mirrors the behavior of pip install for projects with a pyproject.toml; it changes to a separate directory, unpacks the sdist, creates a virtual environment, installs the build dependencies listed in pyproject.toml into that virtual environment, then builds the package. It should definitely be finding NumPy; I'm not sure why it's not finding GEOS.

It does look straightforward to build from the sdist; it might be as simple as changing the checkout step to create an sdist and upload that, then change the download-checkout-artifact steps to download and unpack. Should I try to implement that here?

@molinav
Copy link
Member

molinav commented Feb 4, 2022

@DWesl Then that is the reason for python -m build to be failing. The pipelines are defining GEOS_DIR and NUMPY_INCLUDE_PATH as relative paths to the extern folder where everything external is built in the previous stage:

          export GEOS_DIR=extern
          export NUMPY_INCLUDE_PATH=extern/include
          $env:GEOS_DIR = "extern"
          $env:NUMPY_INCLUDE_PATH = "extern/include"

Once python -m build moves to another folder, these two environment variables are not set correctly anymore and the external libraries are not found. NUMPY_INCLUDE_PATH is particularly important because the previous stage is building headers using NumPy 1.16.6 for Python 2.7 and 3.5-3.9. With NumPy 1.20, an incompatible upgrade came in the expected size of some objects, so if basemap is built with the headers of NumPy 1.20+ and somebody tries to use it later with e.g. NumPy 1.19.5, the basemap extension _geoslib will fail to load. The opposite (compiling with older NumPy headers and later using newer NumPy) does work. All this does not apply for Python 3.10 because the first NumPy supported version is already 1.21.x.

From what I see, I was a bit fast reverting the changes in the basemap-data and basemap-data-hires workflows, since they actually passed. Those could be reimplemented with python -m build if you wish. In the case of basemap, the two exported environment variables would need to be rewritten with absolute paths, and also be sure that the basemap build is done with the older NumPy headers. If python -m build ignores this and picks the latest NumPy version based on the pyproject.toml, then we will have compatibility problems in the generated wheels.

@molinav
Copy link
Member

molinav commented Feb 4, 2022

From a fast check in setup.py, if NUMPY_INCLUDE_PATH is defined, it takes precedence over a potentially-installed numpy:

# Define NumPy include dirs.
numpy_include_path = os.environ.get("NUMPY_INCLUDE_PATH", None)
if numpy_include_path is not None:
include_dirs.append(numpy_include_path)
else:
try:
import numpy
include_dirs.append(numpy.get_include())
except ImportError as err:
warnings.warn("unable to locate NumPy headers", RuntimeWarning)

So your approach with python -m build should also work for the basemap-for-manylinux and basemap-for-windows workflows if GEOS_DIR and NUMPY_INCLUDE_PATH are defined as absolute paths. Would you like to try it?

@DWesl
Copy link
Contributor Author

DWesl commented Feb 4, 2022

Getting an older version of NumPy into the python -m build environment can be done; that's what the oldest-supported-numpy package is for (I'd forgotten about this until you reminded me).

GitHub Actions should be defining a variable with the directory the package gets checked out in, so it should be easy to make the directory variables absolute paths once I find the name of that variable and figure out how to set variables in PowerShell.

@molinav
Copy link
Member

molinav commented Feb 4, 2022

@DWesl The environment variable with the working directory for checkout is GITHUB_WORKSPACE and it comes already as an absolute path.

Thanks for discovering me build, I was doing everything the old-style way. ;-)

@molinav
Copy link
Member

molinav commented Feb 4, 2022

I was checking about the oldest-supported-numpy package, unfortunately I cannot find a version with support for Python 2.7, they all start with at least Python 3.5+:
https://pypi.org/project/oldest-supported-numpy

For a normal new project it would be ok, but there is a lot of legacy code depending on basemap and Python 2.7 should better remain for a while (there was no cp27 wheel files for the first time until last month).

I would therefore prefer to rely on defining NUMPY_INCLUDE_PATH based on the manually-compiled NumPy headers.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 4, 2022

So would oldest-supported-numpy; python_version >= "3.5" and explicit versions for python < 3.5 work? It looks like pyproject.toml understands python_version specifiers since it didn't freak out when I copied the cython version dependencies from requirements-setup.txt.

I'm keeping the NUMPY_INCLUDE_PATH steps in, since deleting it feels like more work.

@molinav
Copy link
Member

molinav commented Feb 4, 2022

@DWesl I think it is a good idea to add the Python version markers in the pyproject.toml file as you say.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 4, 2022

Okay, I'll start pulling over the NumPy versions from the workflow files and the Cython versions from requirements-setup.txt.

@molinav
Copy link
Member

molinav commented Feb 4, 2022

For cython I think you can keep it simple in the pyproject.toml. Using just "cython" should be fine because cython 0.29.x and 3.0.x define python_requires appropriately, so the right version will be picked for Python 2.7 and 3.5+. The explicit cython version definitions are only for pre-historic Python versions, which will not be using pyproject.toml anyhow because they are not supported, but they could be still building from source with the classical approach of calling setup.py.

@molinav
Copy link
Member

molinav commented Feb 4, 2022

I have one question: the current requires in pyproject.toml is only for the build step, right? But it does not affect the installation requirements (i.e. when installing basemap it will also install the newest numpy possible). Is this correct?

If your last changes make the workflows pass, it would be possible to simplify some parts of the YAML files, in particular this:

-
name: Generate NumPy headers
run: |
case "${{ matrix.python-version }}" in
2.6|3.[23]) pkgvers=1.11.3;;
2.7|3.[456789]) pkgvers=1.16.6;;
*) pkgvers=1.21.4;;
esac
pkgname=numpy
pkgcode=numpy-${pkgvers}
python -m pip install cython
python -m pip download --no-binary=numpy "numpy == ${pkgvers}"
unzip ${pkgcode}.zip
rm -f ${pkgcode}.zip
cd ${pkgcode}
python setup.py build
cp build/src*/numpy/core/include/numpy/*.h numpy/core/include/numpy/
cd ..
cp -R ${pkgcode}/numpy/core/include ${{ env.PKGDIR }}/extern
rm -rf ${pkgcode}

and this:

-
name: Generate NumPy headers
run: |
if ("${{ matrix.python-version }}" -In "2.6", "3.2", "3.3") {
Set-Variable -Name "pkgvers" -Value "1.11.3"
} elseif ("${{ matrix.python-version }}" -In "2.7", "3.4", "3.5", "3.6", "3.7", "3.8", "3.9") {
Set-Variable -Name "pkgvers" -Value "1.16.6"
} else {
Set-Variable -Name "pkgvers" -Value "1.21.4"
}
Set-Variable -Name "pkgname" -Value "numpy"
Set-Variable -Name "pkgcode" -Value "numpy-${pkgvers}"
Set-Variable -Name "includedir" -Value "numpy/core/include"
python -m pip install cython
python -m pip download --no-binary=numpy "numpy == ${pkgvers}"
tar -xf "${pkgcode}.zip"
rm "${pkgcode}.zip"
cd "${pkgcode}"
python setup.py build
cp -R build/src.*/${includedir}/numpy/*.h ${includedir}/numpy
cd ..
cp -R "${pkgcode}/${includedir}/numpy" "${{ env.PKGDIR }}/extern/include/numpy"
rm -r "${pkgcode}"

Because all this manual handling of generating the numpy headers would be done internally by python -m build thanks to the latest changes in the pyproject.toml that pin the same numpy versions. Also setting NUMPY_INCLUDE_PATH would not be needed anymore in the workflows (although this possibility can be kept in the setup.py file, it does not harm).

@DWesl
Copy link
Contributor Author

DWesl commented Feb 4, 2022

Correct; the build-system section of the pyproject.toml file only affects the build environment; the run-time/install requirements should remain as they were.

@molinav
Copy link
Member

molinav commented Feb 4, 2022

Some older workflow has failed (Python 3.8) because building NumPy 1.17.3 failed (it looks like for this version we would need to define -std=C99 manually), but it could be fixed by using your NumPy pinning for all the Python versions, since the NumPy versions I picked some time ago are known to work. It was:

  • NumPy 1.16.6 for Python 2.7 and 3.5 to 3.9.
  • NumPy 1.21.4 for Python 3.10.

This would replace the previous oldest-supported-numpy approach for Python 3.5+.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 5, 2022

The new method is (or was, at least) working better on python 2 than python 3. I really did not expect that. I opened pypa/pip#10883 to flag the issue with setup_requires conflicting with an identical build-system.requires.

Comment on lines 166 to 171
cd ${{ env.PKGDIR }}
$env:GEOS_DIR = "extern"
$env:NUMPY_INCLUDE_PATH = "extern/include"
$env:GEOS_DIR = "$env:GITHUB_WORKSPACE/${{ env.PKGDIR }}/extern"
$env:NUMPY_INCLUDE_PATH = "$env:GITHUB_WORKSPACE/${{ env.PKGDIR }}/extern/include"
pip install -r requirements-setup.txt
python setup.py sdist bdist_wheel
python setup.py sdist
pip wheel -w dist --no-deps (Get-Item ${{ env.PKGDIR }}/dist/*.zip)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't the cd ${{ env.PKKGDIR }} on line 166 make the ${{ env.PKGDIR }} in the Get-Item on line 171 unnecessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are totally right. I messed it up there.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 5, 2022

Given that generating from ZIP-format sdists then building a wheel from them keeps running into problems, would it make more sense to revert the wheel build to python setup.py sdist bdist_wheel, then perhaps add a new step to one of the basemap CI jobs to run python -m build just to make sure that isn't broken (or wait until pip fixes the issue with setup_requires and have pip do the install-from-sdist check)?

@molinav
Copy link
Member

molinav commented Feb 5, 2022

@DWesl What about doing python setup.py sdist and then python -m build to generate the wheel? In the end, the first command generates the zip, the second one a tar.gz plus a whl. We can just delete the tar.gz afterwards to be sure that it is not uploaded by mistake to PyPI. The python -m build approach was actually working at some time before.

@molinav
Copy link
Member

molinav commented Feb 5, 2022

@DWesl I was just thinking on my comment before and it is not a good idea, since we can only guarantee that we can build the wheel from the tar but not from the zip, which is what you will find in the end in PyPI when trying to install basemap on Cygwin.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 5, 2022

@DWesl I was just thinking on my comment before and it is not a good idea, since we can only guarantee that we can build the wheel from the tar but not from the zip, which is what you will find in the end in PyPI when trying to install basemap on Cygwin.

It would appear pip refuses to build a wheel from the .tar.gz sdists, even ones where build succeeds. Since it keeps complaining about not being able to meet the requirements for 3.2, I put some code in setup.py to drop those from setup_requires; they'll still be around in requirements-setup.txt if someone tries to install on python3.2, but they shouldn't cause pip problems on python > 3.4.

@molinav
Copy link
Member

molinav commented Feb 5, 2022

It seems that having setup_requires around was not a good idea, I didn't even know it was deprecated.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 5, 2022

It seems that having setup_requires around was not a good idea, I didn't even know it was deprecated.

It was the best way to specify build-time dependencies until pyproject.toml came along (PEP 517 was accepted about five years ago) and got widespread uptake (pip didn't implement support for it until 2019). Since it seems you're trying to keep compatibility with python back to 2.6 and 3.2, I suspect you'll want to keep it around.

@molinav
Copy link
Member

molinav commented Feb 5, 2022

The limits for the Python versions are not that strict I would say. I took these requirement files from some templates that I have that would fit for basemap, and these templates tried to cover all the versions I needed to track down in the past.

I think it would be safe e.g. to remove the references to Python 3.2, since it brings other problems (pip < 8 therefore no manylinux wheel support, the basemap code still uses the u"" syntax somewhere and this is not compatible in Python 3.0-3.2).

I think Python 2.6-2.7 and 3.5+ is a good deal, Python 2.6 is not difficult to maintain because basemap uses the old syntax for string formatting. But you never know what you may find in a server (I still see Python 3.4 in some places).

@molinav molinav merged commit 5a2d098 into matplotlib:develop Feb 5, 2022
@molinav
Copy link
Member

molinav commented Feb 5, 2022

@DWesl I just merged the PR. Could you check if you can build from the sdist in your environment (Windows with Cygwin)? You can pick the zip file from the artifacts stored in the GitHub Actions.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 6, 2022

$ unzip artifacts-build-x86-2.7.zip
Archive:  artifacts-build-x86-2.7.zip
  inflating: basemap-1.3.1+dev-cp27-cp27mu-linux_i686.whl
  inflating: basemap-1.3.1+dev-cp27-cp27mu-manylinux1_i686.whl
  inflating: basemap-1.3.1+dev.zip

$ for ver in 2.7 3.{5,6,7,8,9}; 
> do 
>     python${ver} -m pip install basemap-1.3.1+dev.zip
> done
DEPRECATION: Python 2.7 reached the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 is no longer maintained. pip 21.0 will drop support for Python 2.7 in January 2021. More details about Python 2 support in pip can be found at https://pip.pypa.io/en/latest/development/release-process/#python-2-support pip 21.0 will remove support for this functionality.
...
Building wheels for collected packages: basemap
  Building wheel for basemap (PEP 517) ... done
  Created wheel for basemap: filename=basemap-1.3.1+dev-cp27-cp27m-cygwin_3_3_4_x86_64.whl size=207644 sha256=b13ceea5af1eb639af8342f4d3230ab9577b65621038940e39cc8418618abe0d
  Stored in directory: ${HOME}/.cache/pip/wheels/18/61/bc/04286efe1a42778576b35bebbf76986ee8939cecf85e357ecf
Successfully built basemap
...
Successfully installed basemap-1.3.1+dev
DEPRECATION: Python 3.5 reached the end of its life on September 13th, 2020. Please upgrade your Python as Python 3.5 is no longer maintained. pip 21.0 will drop support for Python 3.5 in January 2021. pip 21.0 will remove support for this functionality.
...
Building wheels for collected packages: basemap
  Building wheel for basemap (PEP 517) ... done
  Created wheel for basemap: filename=basemap-1.3.1+dev-cp35-cp35m-cygwin_3_3_4_x86_64.whl size=204099 sha256=b74dcd2ed16675e76dc5089809369156e24a7d597117334b561ddd6b0081d9ba
  Stored in directory: ${HOME}/.cache/pip/wheels/7a/1b/3f/4df0af4d794f0daf480be0e561be949bb26f5470f5d03bdb8f
Successfully built basemap
...
Successfully installed basemap-1.3.1+dev six-1.15.0
...

I think it works

@DWesl DWesl deleted the build-from-sdist branch February 6, 2022 12:22
@molinav
Copy link
Member

molinav commented Feb 10, 2022

@DWesl May I ask you to try one last time using the latest sdist available in the GitHub Actions? It is labelled as 1.3.2. I made slight changes after the PR and would like to be sure that everything works for you before publishing to PyPI.

@DWesl
Copy link
Contributor Author

DWesl commented Feb 10, 2022

$ python -m pip install basemap-1.3.2.zip
Defaulting to user installation because normal site-packages is not writeable
Looking in links: /usr/share/python-wheels
Processing ./basemap-1.3.2.zip
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Requirement already satisfied: numpy<1.23,>=1.21 in ${HOME}/.local/lib/python3.9/site-packages (from basemap==1.3.2) (1.21.5)
Requirement already satisfied: pyproj<3.4.0,>=1.9.3 in /usr/lib/python3.9/site-packages (from basemap==1.3.2) (3.3.0)
Requirement already satisfied: matplotlib<3.6,>=1.5 in /usr/lib/python3.9/site-packages (from basemap==1.3.2) (3.5.1)
Requirement already satisfied: pyshp<2.2,>=1.2 in ${HOME}/.local/lib/python3.9/site-packages (from basemap==1.3.2) (2.1.3)
Requirement already satisfied: basemap-data<1.4,>=1.3 in /usr/lib/python3.9/site-packages (from basemap==1.3.2) (1.3.0)
Requirement already satisfied: six<1.16,>=1.10 in ${HOME}/.local/lib/python3.9/site-packages (from basemap==1.3.2) (1.15.0)
Requirement already satisfied: cycler>=0.10 in /usr/lib/python3.9/site-packages (from matplotlib<3.6,>=1.5->basemap==1.3.2) (0.11.0)
Requirement already satisfied: python-dateutil>=2.7 in /usr/lib/python3.9/site-packages (from matplotlib<3.6,>=1.5->basemap==1.3.2) (2.8.2)
Requirement already satisfied: fonttools>=4.22.0 in /usr/lib/python3.9/site-packages (from matplotlib<3.6,>=1.5->basemap==1.3.2) (4.28.5)
Requirement already satisfied: kiwisolver>=1.0.1 in /usr/lib/python3.9/site-packages (from matplotlib<3.6,>=1.5->basemap==1.3.2) (1.3.2)
Requirement already satisfied: pyparsing>=2.2.1 in /usr/lib/python3.9/site-packages (from matplotlib<3.6,>=1.5->basemap==1.3.2) (3.0.6)
Requirement already satisfied: packaging>=20.0 in /usr/lib/python3.9/site-packages (from matplotlib<3.6,>=1.5->basemap==1.3.2) (21.3)
Requirement already satisfied: pillow>=6.2.0 in /usr/lib/python3.9/site-packages (from matplotlib<3.6,>=1.5->basemap==1.3.2) (9.0.0)
Requirement already satisfied: certifi in /usr/lib/python3.9/site-packages (from pyproj<3.4.0,>=1.9.3->basemap==1.3.2) (2021.10.8)
Building wheels for collected packages: basemap
  Building wheel for basemap (pyproject.toml) ... done
  Created wheel for basemap: filename=basemap-1.3.2-cp39-cp39-cygwin_3_3_4_x86_64.whl size=235019 sha256=fcb6645099406b4d65a107325175e307d95ccc038c59ac39b978be5725d612ee
  Stored in directory: ${HOME}/.cache/pip/wheels/3e/8c/72/1d6fd45bb09223f56e9c1d6845fce9c2f6e4f14ae77252eb2e
Successfully built basemap
Installing collected packages: basemap
  Attempting uninstall: basemap
    Found existing installation: basemap 1.3.1+dev
    Uninstalling basemap-1.3.1+dev:
      Successfully uninstalled basemap-1.3.1+dev
Successfully installed basemap-1.3.2

Looks like it works.

@molinav
Copy link
Member

molinav commented Feb 10, 2022

@DWesl Thanks for the effort of the pull request and the testing. basemap 1.3.2 is finally available in PyPI with working sdist file:

https://pypi.org/project/basemap/1.3.2/#files

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.

2 participants