Modernize package by valeriupredoi · Pull Request #69 · NCAS-CMS/pyfive

valeriupredoi · 2025-02-07T15:39:11Z

Hi @jjhelmus in addition to @bnlawrence's #68 I have put together a PR that is updating the pyfive package to fairly modern standards, by:

adding a conda env file
pyproject file
decent GHA workflow
potential for registering with codecov and displaying cov CI results via that app (in eg PRs)

and a few other small bits and bobs. Please let me know what you think - and many thanks, and a good weekend 🍺

kalvdans · 2025-02-07T15:48:32Z

pyproject.toml looks nice and great to remove setup.py, moving to a declarative package description instead of a script. 👍

I'll let others review the other files.

valeriupredoi · 2025-02-07T15:53:11Z

many thanks @kalvdans 🍻

jjhelmus · 2025-02-07T15:54:12Z

pyproject.toml

+dependencies = [
+    "numpy",
+    "pip!=21.3",
+    "s3fs",


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?

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?

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?

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 👍

done in 35476b7

kmuehlbauer

Thanks @valeriupredoi for tackling this. Just two comments from my side.

kmuehlbauer · 2025-02-10T06:50:58Z

.github/workflows/pytest.yml

+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+      - uses: conda-incubator/setup-miniconda@v3


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.

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 👍

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

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 👍

kmuehlbauer · 2025-02-10T06:52:15Z

.github/workflows/pytest.yml

+      - name: Install Pyfive Test Mode
+        run: |
+          pip install -e .[test]
+      - name: Lint with flake8


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.

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?

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 👍

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

valeriupredoi · 2025-02-10T12:06:23Z

many thanks for the review @kmuehlbauer 🍺

jjhelmus · 2025-02-10T21:54:15Z

.github/workflows/pytest.yml

      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"]


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

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 👍

Python 3.9 re-added in 32158d4 and f2e9bcb 👍

jjhelmus · 2025-02-10T21:55:43Z

pyproject.toml

+]
+dependencies = [
+    "numpy",
+    "pip!=21.3",


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.

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)

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)

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

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).

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.

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 👍

pyproject.toml

jjhelmus · 2025-02-10T22:26:57Z

I don't think the python-version variable is coming into play in the CI with the switch to setup-miniconda. Looking at the test runs they all seem to be running Python 3.12:

For example the Test Python 3.10 run:

Run pytest -n 2
============================= test session starts ==============================
platform linux -- Python 3.12.3, pytest-[8](https://github.com/jjhelmus/pyfive/actions/runs/13203985085/job/36866023029?pr=69#step:7:9).3.4, pluggy-1.5.0
rootdir: /home/runner/work/pyfive/pyfive
configfile: pyproject.toml
plugins: html-4.1.1, xdist-3.6.1, metadata-3.1.1, cov-6.0.0
created: 2/2 workers
2 workers [41 items]

.........................................                                [[10](https://github.com/jjhelmus/pyfive/actions/runs/13203985085/job/36866023029?pr=69#step:7:11)0%]
- Generated html report: file:///home/runner/work/pyfive/pyfive/test-reports/report.html -
============================== 41 passed in 0.66s ==============================

valeriupredoi · 2025-02-10T23:04:24Z

Blast! I know what it is, I forgot to set the default run shell at the top of the workflow - without it it'll always ignore any active environment 😮‍💨

valeriupredoi · 2025-02-10T23:14:18Z

I don't think the python-version variable is coming into play in the CI with the switch to setup-miniconda. Looking at the test runs they all seem to be running Python 3.12:

For example the Test Python 3.10 run:

Run pytest -n 2
============================= test session starts ==============================
platform linux -- Python 3.12.3, pytest-[8](https://github.com/jjhelmus/pyfive/actions/runs/13203985085/job/36866023029?pr=69#step:7:9).3.4, pluggy-1.5.0
rootdir: /home/runner/work/pyfive/pyfive
configfile: pyproject.toml
plugins: html-4.1.1, xdist-3.6.1, metadata-3.1.1, cov-6.0.0
created: 2/2 workers
2 workers [41 items]

.........................................                                [[10](https://github.com/jjhelmus/pyfive/actions/runs/13203985085/job/36866023029?pr=69#step:7:11)0%]
- Generated html report: file:///home/runner/work/pyfive/pyfive/test-reports/report.html -
============================== 41 passed in 0.66s ==============================

fixed in 8de5832 - that was a sneaky one, great eye for it! 🍺

Co-authored-by: Jonathan J. Helmus <jjhelmus@gmail.com>

valeriupredoi · 2025-02-10T23:29:58Z

Python 3.9 re-added in 32158d4 and f2e9bcb 👍

valeriupredoi · 2025-02-11T16:07:41Z

also relevant to dropping support for Python 3.9 (and 3.10) is SPEC0 if you folks think about subscribing to it 👍

valeriupredoi · 2025-02-17T16:37:27Z

@jjhelmus what do you reckon, mate - this one's good for merging? 🍺

jjhelmus · 2025-02-17T19:40:21Z

As mentioned in #51, I want to get other involved in this project. I cannot be the only one reviewing and merging PR. Ideally I wouldn't need to do either.

If someone else can review and merge this PR it would be appreciated.

valeriupredoi · 2025-02-18T19:56:36Z

sounds like a solid plan to me, cheers @jjhelmus 🍺 @kmuehlbauer @bmaranville @davidhassell any of you good folk has a minute to have a final review and merge - if all's in order, please? 🍻

kmuehlbauer · 2025-02-19T11:20:50Z

pyproject.toml

+
+[tool.coverage.run]
+parallel = true
+source = ["esmvalcore"]


esmvalcore does not fit here.

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

kmuehlbauer

I think this PR has been discussed extensively and suggestions have been incorporated.

I found one more item which needs change, but otherwise this is good to go.

valeriupredoi · 2025-02-19T12:19:09Z

very many thanks @kmuehlbauer - fixed that little wiggle :beer

kmuehlbauer · 2025-02-19T12:26:01Z

Thanks @valeriupredoi.

valeriupredoi · 2025-02-19T12:28:30Z

wonderful, great many thanks @kmuehlbauer and everyone else 🍻

valeriupredoi added 16 commits February 7, 2025 14:38

remove old setup files

881b72b

add conda env file

69e4289

add to gitignore

4906a05

add pyproject toml file

fe0faad

add pip dep

9b02d4e

add pip and flake8 deps

81d1926

update GHA

0c6063a

change GHA name

c473862

reformat GHA wkflow

0991ccf

add python pin

f007c46

add test-reports to gitignore

f0a6aab

add codecov config

3f90d33

add coverage settings

7191523

run conda list

6931182

check env

ecab80c

add note

b055769

jjhelmus reviewed Feb 7, 2025

View reviewed changes

reduce s3fs to test dep

35476b7

kmuehlbauer approved these changes Feb 10, 2025

View reviewed changes

valeriupredoi added the installation label Feb 10, 2025

jjhelmus reviewed Feb 10, 2025

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

set shell entrypoint default

8de5832

valeriupredoi and others added 3 commits February 10, 2025 23:23

Update pyproject.toml

4b6da71

Co-authored-by: Jonathan J. Helmus <jjhelmus@gmail.com>

add py3.9 to GHA

32158d4

change python pin in env

f2e9bcb

valeriupredoi added 4 commits February 10, 2025 23:39

readd py 39 in package metadata

be32c82

remove pip as conda forge install dep

5826cfa

remove pip as pip install dep

788026d

install pip in GHA

d911a7f

kmuehlbauer reviewed Feb 19, 2025

View reviewed changes

kmuehlbauer requested changes Feb 19, 2025

View reviewed changes

remove reference to other package

62a5b39

kmuehlbauer approved these changes Feb 19, 2025

View reviewed changes

kmuehlbauer merged commit bfe1a21 into NCAS-CMS:master Feb 19, 2025
5 checks passed

valeriupredoi deleted the modernize_package branch February 19, 2025 12:28

Conversation

valeriupredoi commented Feb 7, 2025

Uh oh!

kalvdans commented Feb 7, 2025

Uh oh!

valeriupredoi commented Feb 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmuehlbauer left a comment

Choose a reason for hiding this comment

Uh oh!

kmuehlbauer Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valeriupredoi commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jjhelmus commented Feb 10, 2025

Uh oh!

This comment was marked as outdated.

valeriupredoi commented Feb 10, 2025

Uh oh!

valeriupredoi commented Feb 10, 2025

Uh oh!

valeriupredoi commented Feb 10, 2025

Uh oh!

valeriupredoi commented Feb 11, 2025

Uh oh!

valeriupredoi commented Feb 17, 2025

Uh oh!

jjhelmus commented Feb 17, 2025

Uh oh!

valeriupredoi commented Feb 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmuehlbauer Feb 10, 2025 •

edited

Loading