Modernise setup by moving from setup.py to pyproject.toml#438
Modernise setup by moving from setup.py to pyproject.toml#438observingClouds wants to merge 29 commits intoecmwf:masterfrom
Conversation
observingClouds
changed the title
DWesl
left a comment
DWesl
left a comment
There was a problem hiding this comment.
Looks good overall, although I'm curious about the combined shift from setup.py to pyproject.toml and from setuptools to hatchling.
Are there any benefits to hatchling over setuptools beyond hatchling's integration with hatch?
| license="Apache License Version 2.0", | ||
| url="https://github.com/ecmwf/cfgrib", | ||
| packages=setuptools.find_packages(), | ||
| include_package_data=True, |
There was a problem hiding this comment.
This setting doesn't get carried over and could, but on the other hand, I don't think there's actually any package data files to include, so dropping this might be the right thing to do.
There was a problem hiding this comment.
I implemented a packaging tests that runs the test suite against the wheel and sdist to ensure that nothing is missing from the package. Now unnecessary files, like tests, grib and netCDF files are no longer included in the wheel, dropping the size of the package from around 9MB to less than 50KB 🥳. So, yes include_package_data is not necessary as the tests show.
| @@ -1,5 +1,66 @@ | |||
| [build-system] | |||
| requires = [ "setuptools>=61", "setuptools-scm>=8" ] | |||
There was a problem hiding this comment.
There is a hatch VCS version plugin, though the old setup.py specified version, so I'm not sure setuptools_scm's versioner actually got used: I certainly don't see the configuration required to get the version in the sdist and wheel filenames with the old version.
This might be why .pre-commit-config.yaml got dropped from the sdist, which is the big difference between the created files (Setuptools-scm sorts alphabetically and includes entries for each directory, while hatch sorts files before directories, except for a list of five specific files, but that should be a minor difference)
EDIT: I just realized: if you split off the hatchling change and try to keep the current version determination, I'm not sure if the setuptools version or the setuptools_scm version will wind up getting used. (EDIT: If a version is specified in the project table, even dynamically, that one is used. If there are additionally directions on how setuptools_scm should get a version, it warns that those directions won't be used).
There was a problem hiding this comment.
The version seems to have always been extracted from cfgrib/__init__.py. The new format just does not require the boilerplate code, as this is integrated into hatch
…nto maint/pyproject
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
1 similar comment
|
Caution This pull request contains changes to GitHub workflows! |
observingClouds
force-pushed
the
maint/pyproject
branch
from
93bee42 to
0493f48
Compare
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
datetime64/timedelta64 need to be casted to int32 as netCDF3 (used by scipy) does not support int64
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
|
Caution This pull request contains changes to GitHub workflows! |
DWesl
left a comment
DWesl
left a comment
There was a problem hiding this comment.
I like the move from setup.py to pyproject.toml, and think that part of this PR is in a good place.
I kind of understand the move from setuptools to hatchling, though I would have left that for a second PR. I think that part also works.
Dropping the requirements*.txt files listing the versions of each Python package used in testing is a choice. The primary relation to the rest of the PR is that the requirements*.in files got moved to sections of pyproject.toml.
There's a collection of CI changes that would probably work better as a separate PR, to show that the tests pass with the current setup and also that this PR doesn't break those tests.
Python 3.9 and all those before it have reached end of support, so it's probably time to drop support for those versions, but I don't know that a PR about moving from setup.py to pyproject.toml and setuptools to hatchling is the place to do it.
I don't see a replacement for the tox functionality that was removed: does uv have something similar? Is there a way to run GitHub Actions from the command line?
| activate-environment: ${{ matrix.os }}-${{ matrix.python }}${{ matrix.extras }} | ||
| environment-file: environment${{ matrix.extras }}.in.yml | ||
| - name: Export concrete dependencies | ||
| shell: bash -l {0} | ||
| run: | | ||
| conda env export --no-build -f tests/environment-${{ matrix.os }}-${{ matrix.python }}${{ matrix.extras }}.yml | ||
| git diff |
There was a problem hiding this comment.
This, I think, would be a uv compile step
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: conda-incubator/setup-miniconda@v2 | ||
| - uses: astral-sh/setup-uv@v5 |
There was a problem hiding this comment.
Have you confirmed that uv can handle installing the ecCodes C library in addition to the ecCodes python package?
| unreleased | ||
| ---------- | ||
|
|
||
| - Deprecating setuptools in favor of hatchling and pyproject.toml. |
There was a problem hiding this comment.
Nothing you do here will deprecate setuptools, which is still a valid build backend (unlike distutils, which was deprecated some years ago and has since been removed from Python). Among other things, setuptools is one of three backends that supports compiled extension modules. Hatchling, so far, only seems to support Cython extension modules.
I would then suggest:
| - Deprecating setuptools in favor of hatchling and pyproject.toml. | |
| - Move away from discouraged `setup.py` in favor of `pyproject.toml`, | |
| and also replace setuptools with hatchling as the build backend |
and then this PR does more things, which might need to be documented in the changelog:
- overhaul CI test setup to drop Appveyor in favor of GitHub Actions
- set default encoding for time variables
- drop support and testing for Python<3.10
- drop tox configuration
| - Fixed xarray.backends imports for compatibilty with recent versions of xarray. | ||
| See `#409 <https://github.com/ecmwf/cfgrib/pull/409>`_. | ||
|
|
||
| - Set default dtype for step to `timedelta64[ns]` for compatibilty with | ||
| future versions of xarray. | ||
| See `#427 <https://github.com/ecmwf/cfgrib/pull/427>`_. | ||
|
|
||
| - Fixed xarray.backends imports for compatibilty with recent versions of xarray. | ||
| See `#409 <https://github.com/ecmwf/cfgrib/pull/409>`_. | ||
|
|
There was a problem hiding this comment.
Is there a reason you changed the order?
| # NOTE: label based indexing in xarray is inclusive of both the start and stop bounds. | ||
| assert da.sel(number=slice(2, 6)).mean() == va[2:7].mean() | ||
| assert da.sel(number=slice(2, 6, 2)).mean() == va[2:7:2].mean() | ||
| assert np.isclose(da.sel(number=slice(2, 6, 2)).mean(), va[2:7:2].mean()) |
There was a problem hiding this comment.
Could you elaborate on why this was changed? Those two expressions should be performing exactly the same operations: averaging the third, fifth, and seventh elements of the array used to form the DataArray; there is an excellent chance XArray forwards to the NumPy method used on the right.
In addition, pytest has an approx function to do what you use np.isclose for here (that is, `da.sel(...).mean() == approx(va[...].mean(), ...)
| [tox] | ||
| envlist = qc, docs, py37, pypy3, deps | ||
|
|
||
| [testenv] | ||
| passenv = WHEELHOUSE PIP_FIND_LINKS PIP_WHEEL_DIR PIP_INDEX_URL | ||
| setenv = PYTHONPATH = {toxinidir} | ||
| deps = -r{toxinidir}/ci/requirements-tests.txt | ||
| commands = pytest -v --flakes --cache-clear --basetemp={envtmpdir} {posargs} | ||
|
|
||
| [testenv:docs] | ||
| deps = -r{toxinidir}/ci/requirements-docs.txt | ||
| commands = sphinx-build -W -b html docs build/sphinx/html | ||
|
|
||
| [testenv:qc] | ||
| basepython = python3.7 | ||
| # needed for pytest-cov | ||
| usedevelop = true | ||
| commands = pytest -v --flakes --pep8 --mccabe --cov=cfgrib --cov=cf2cdm --doctest-glob="*.rst" --cov-report=html --cache-clear --basetemp={envtmpdir} {posargs} | ||
|
|
||
| [testenv:deps] | ||
| deps = | ||
| commands = python setup.py test |
There was a problem hiding this comment.
Could you elaborate on why you got rid of the tox configuration? The dependencies would look a little different, but the ability to run all tests and linters with a single command is useful, especially for beginning contributors who don't know what to look for.
DWesl
left a comment
DWesl
left a comment
There was a problem hiding this comment.
The CI overview informs me that the CLA check is failing: have you completed and submitted the Contributor License Agreement? (Can one of the maintainers point to where the docs show where that is?)
You would also need to edit CONTRIBUTING.rst to reflect that tox will no longer run tests in a collection of Python environments and also run the linters and build the documentation. Editing the Makefile might work, but I'm not sure how many Python developers have make.
| if "datetime64" in dtype_str: | ||
| netcdf_kwargs["encoding"][var_name] = { | ||
| "units": "seconds since 1970-01-01", | ||
| "dtype": "int32", |
There was a problem hiding this comment.
Out of curiosity, do you know what the GRIB format uses for these variables?
| uses: actions/checkout@v6 | ||
| with: | ||
| sparse-checkout: | | ||
| tox.ini |
2 participants
Description
This PR modernizes the setup of cfgrib via pyproject.toml, so that one can also use modern tools like
uvduring e.g. development.This includes the following changes:
setuptoolstohatchfor good reasonstoxtohatchto simplify setuppyproject.tomlPlease excuse this rather large PR. The setup was quite antique (e.g. using
xarray==0.15.1and python 3.7), so quite a few changes were necessary. Nevertheless, tests should cover all the changes and improved the overall stability of this project.Contributor Declaration
By opening this pull request, I affirm the following: