-
Notifications
You must be signed in to change notification settings - Fork 22
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
Changes from 16 commits
881b72b
69e4289
4906a05
fe0faad
9b02d4e
81d1926
0c6063a
c473862
0991ccf
f007c46
f0a6aab
3f90d33
7191523
6931182
ecab80c
b055769
35476b7
8de5832
4b6da71
32158d4
f2e9bcb
be32c82
5826cfa
788026d
d911a7f
62a5b39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"] | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! I must have overlooked the |
||
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 |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
coverage: | ||
status: | ||
project: off | ||
patch: | ||
default: | ||
target: 100% |
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 |
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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, but other of its (possible future) dependencies may eg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this listed as a dependency There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I hope, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
"s3fs", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also I don't usually see a pip version in the dependencies - is that needed too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nevermind me, I double-checked #68 and There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
valeriupredoi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
[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"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:", | ||
] |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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 👍