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

Modernize package #69

Merged
merged 26 commits into from
Feb 19, 2025
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 32 additions & 23 deletions .github/workflows/pytest.yml
Original file line number Diff line number Diff line change
@@ -1,41 +1,50 @@
# This workflow will install Python dependencies, run tests and lint with a variety of Python versions
# For more information see: https://help.github.com/actions/language-and-framework-guides/using-python-with-github-actions

name: Python package
name: Pyfive Test

on:
push:
branches: [ master ]
branches: [ master, modernize_package ] # leave only "master" after PR69 is merged
pull_request:
branches: [ master ]
schedule:
- cron: '0 0 * * *' # nightly

jobs:
build:

name: Test Python ${{ matrix.python-version }}
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
python-version: ["3.8", "3.9", "3.10", "3.11", "3.12", "3.13"]
python-version: ["3.10", "3.11", "3.12", "3.13"]
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a compelling reason to drop Python 3.9? Is is supported until the end of Oct 2025 (https://endoflife.date/python)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed! But a lot of other packages are dropping it like a hot potato - I don't think it's feasible to support it anymore (even if Pyfive has a very small dependency table). Though it's not make or break to me, so I can re-add it, and open an issue to remind us to remove it in eight months 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Python 3.9 re-added in 32158d4 and f2e9bcb 👍


steps:
- uses: actions/checkout@v3
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v3
with:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
run: |
python -m pip install --upgrade pip
python -m pip install flake8 pytest
python -m pip install -e .
if [ -f requirements.txt ]; then pip install -r requirements.txt; fi
- name: Lint with flake8
run: |
# stop the build if there are Python syntax errors or undefined names
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
- name: Test with pytest
run: |
pytest
- uses: actions/checkout@v4
with:
fetch-depth: 0
- uses: conda-incubator/setup-miniconda@v3
Copy link
Collaborator

@kmuehlbauer kmuehlbauer Feb 10, 2025

Choose a reason for hiding this comment

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

We might be able to reduce the setup time using https://github.com/mamba-org/setup-micromamba. Works quite well over at h5netcdf and xarray. But I'm fine with the proposed, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good point! We're using setup-miniconda with mamba in a lot of our projects and I know the folks there at conda-incubator too, that's why I used it here too. I reckon we can change to micromamba in the future, if we hit any performance issues 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, sure, my main point is minimizing the CI time. Less time, less energy consumption.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same with us, in stark opposition to the Techbro movement 😁 This one's lightning fast too, it'll be even faster once they update the base mamba version, for which I already opened an issue 👍

with:
activate-environment: pyfive
environment-file: environment.yml
python-version: ${{ matrix.python-version }}
miniforge-version: "latest"
use-mamba: true
- name: Install Pyfive Test Mode
run: |
pip install -e .[test]
- name: Lint with flake8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to split out linting in a separate job? For the packages I maintain I find it useful to see test breakages even if the linting fails.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's entirely up to you, folks! We stop and fail the workflow if eg pre-commit fails, but I guess that depends how much emphasis one project puts on linting. Eg here we can put a fail-fast: false clause so the workflow still fails but it runs all the steps without halting at the first failed step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just noticed that the strategy already has a fail-fast: false so this would mean workflow runs all the steps and marks the wkflow as failed if one step failed, so we good here I reckon 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! I must have overlooked the fail-fast setting.

run: |
# stop the build if there are Python syntax errors or undefined names
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics
- name: Check environment contents
run: |
conda list
pip list
- name: Test with pytest
run: |
pytest -n 2
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,6 @@
.coverage
build
pyfive.egg-info/
pyfive/__pycache__/
tests/__pycache__/
test-reports
6 changes: 6 additions & 0 deletions codecov.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
coverage:
status:
project: off
patch:
default:
target: 100%
11 changes: 11 additions & 0 deletions environment.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
name: pyfive
channels:
- conda-forge
- nodefaults

dependencies:
- numpy
- pip !=21.3
- python >=3.10
- s3fs
105 changes: 105 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
[build-system]
requires = [
"setuptools >= 40.6.0",
"setuptools_scm>=6.2",
]
build-backend = "setuptools.build_meta"

[project]
authors = [
{name = "Pyfive Development Team", email = "jjhelmus@gmail.com"}
]
classifiers = [
"Development Status :: 5 - Production/Stable",
"Environment :: Console",
"Intended Audience :: Developers",
"Intended Audience :: Science/Research",
"License :: OSI Approved :: BSD License",
"Natural Language :: English",
"Operating System :: POSIX :: OS Independent",
"Programming Language :: Python :: 3",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
"Programming Language :: Python :: 3.12",
"Programming Language :: Python :: 3.13",
"Topic :: Scientific/Engineering",
"Topic :: Scientific/Engineering :: Atmospheric Science",
"Topic :: Scientific/Engineering :: Physics",
]
dynamic = [
"readme",
"version",
]
dependencies = [
"numpy",
"pip!=21.3",
Copy link
Owner

Choose a reason for hiding this comment

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

pip should not be included as a package dependency. It is a front end that can be used to install pyfive but it is not needed at runtime.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tend to agree with you on this one, but the problem is that it becomes murky which pip is used where and by what - we should avoid using pip default from PyPI when working with conda packages. If you are 100% that this package will only need numpy as a build dependency, then we can remove pip here, but if Pyfive will start adding a number of new build dependencies from conda-forge then we should most deffo use the conda-forge pip (and have it as a build/install dependency)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a huge fan of conda-forge, but pyfive is a pure-Python package and I'm hoping it won't need to be built (or have build dependencies)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, but other of its (possible future) dependencies may eg netCDF4 - getting those off PyPI is a bit of a pain 😁

Copy link
Owner

Choose a reason for hiding this comment

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

With this listed as a dependency pip will be installed into the environment from PyPI by the pip installed by conda but only if the upgrade strategy is eager (not the default) or something else required a different version of pip. In the current run the conda installed version is retained. I can't see any case where this would be needed, the packages listed here are installed by pip itself (or another PEP 517 front end).

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, but other of its (possible future) dependencies may eg netCDF4 - getting those off PyPI is a bit of a pain 😁

I hope, pyfive will never depend on netCDF4 as this depends on netcdf-c which depends on hdf5 which counters the efforts of pyfive, IMHO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah no way, that was a somewhat anecdotal example 😃 Let me just remobe pip from the conda env, then - so that numpy is the only one left as install dep 👍

"s3fs",
Copy link
Owner

Choose a reason for hiding this comment

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

Is this a run dependency or is this similar to tests? If the later would a test extra be a better location for this dependency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hi @jjhelmus it's a run dep needed via #68 see https://github.com/jjhelmus/pyfive/pull/68/files#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52 - I can remove it here, and re-add it once if/when #68 gets merged?

Copy link
Collaborator

Choose a reason for hiding this comment

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

s3fs is a surprising dependency. Does this have to be installed to use pyfive in the way the majority of users use it (without lazy-loading chunks?)

Also I don't usually see a pip version in the dependencies - is that needed too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nevermind me, I double-checked #68 and s3fs is indeed a simple test dep like all the others like h5netcdf etc, am gonna move it there right now 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in 35476b7

]
description = "A pure python HDF5 reader"
license = {text = "BSD License, Version 3-Clause"}
name = "pyfive"
requires-python = ">=3.10"

[project.optional-dependencies]
test = [
"pytest>6.0.0",
"pytest-cov>=2.10.1",
"pytest-html!=2.1.0",
"pytest-xdist",
"dask",
"flake8",
"flask",
"flask-cors",
"h5netcdf",
"h5py",
"netCDF4",
"moto",
]

# very possible these will be needed
# at some point in the near future
# doc = [
# "sphinx>=6.1.3",
# "pydata_sphinx_theme",
# ]
# develop = [
# "pre-commit",
# "pylint",
# "pydocstyle",
# ]


[project.urls]
Code = "https://github.com/jjhelmus/pyfive"
# Documentation = ""
Issues = "https://github.com/jjhelmus/pyfive/issues"

[tool.setuptools]
include-package-data = true
license-files = ["LICENSE"]
packages = ["pyfive"]
zip-safe = false # possibly yes

[tool.setuptools.dynamic]
readme = {file = "README.md", content-type = "text/markdown"}

[tool.setuptools_scm]
version_scheme = "release-branch-semver"

[tool.pytest.ini_options]
addopts = [
"--cov-report=xml:test-reports/coverage.xml",
"--cov-report=html:test-reports/coverage_html",
"--html=test-reports/report.html",
]

[tool.coverage.run]
parallel = true
source = ["esmvalcore"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

esmvalcore does not fit here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my bad, inspiration for the file from another of our packages 😁 Fixed in 62a5b39


[tool.coverage.report]
exclude_lines = [
"pragma: no cover",
"if __name__ == .__main__.:",
"if TYPE_CHECKING:",
]
2 changes: 0 additions & 2 deletions setup.cfg

This file was deleted.

41 changes: 0 additions & 41 deletions setup.py

This file was deleted.