Skip to content

Conversation

bcmyguest
Copy link

@bcmyguest bcmyguest commented Aug 24, 2025

Description Of Changes

Closes #3847

  • Updates the min version to python 3.11 for the package
  • Github workflows test earliest version and without extras for 3.11 instead of 3.10

Not relevant to this MR, but may be nice to have in future (and I would be happy to add if there's interest):

  • pre-commit hooks for formatting (inc line endings, ruff, etc.)
  • migrate to uv? This may be controversial - happy to drop it. Reasons to do so would be a) faster install time b) locked dependencies (easier to manage and don't break randomly) c) easier to upgrade dependencies (uv lock --upgrade). Reasons not to a) Some devs may prefer/need conda b) I'm not familiar enough with the package to see other flaws.
  • Remove setup.cfg, move to using pyproject.toml fully.
  • Migrate fully to ruff (I know this may not be fully possible now due to flake8-continuation warnings & maintenance #3866)
  • parallelize tests (xdist?)

Hope this is not overstepping, feel free to discard this PR. Have a nice day!

Checklist

  • Closes #xxxx
  • Tests added -> N/A
  • Fully documented -> I think so

@bcmyguest bcmyguest requested a review from a team as a code owner August 24, 2025 02:21
@bcmyguest bcmyguest requested review from dopplershift and removed request for a team August 24, 2025 02:21
@CLAassistant
Copy link

CLAassistant commented Aug 24, 2025

CLA assistant check
All committers have signed the CLA.

@bcmyguest
Copy link
Author

bcmyguest commented Aug 24, 2025

Need to upgrade pyproj version. It's being built from source in the pipeline due to the Python version change (or setup so that builds from source work on older pyproj).

@dcamron
Copy link
Member

dcamron commented Aug 25, 2025

You are absolutely not overstepping, thanks for opening this! We're aiming to cut a bug fix release in the coming weeks, and I'll confirm with @dopplershift about saving this for after or not. Also, looks like SciPy >= v1.9.2 (2022) gives us Python 3.11 wheels if we don't want to deal with these build failures on minimum. Otherwise we gotta plumb in libopenblas I think.

I doubt you've missed anything here, but I'll take a proper look later this week. In the meantime, glad to shout out a few of your points!


pre-commit hooks for formatting (inc line endings, ruff, etc.)

We've avoided this, or taking on a formatter, up to this point, and I think has historically benefitted our community of scientists/researchers/students, often first time PR authors. Instead we opt for the "document, describe, offer feedback" approach to avoid any additional dev setup and potential barriers to contributing. It does make the review and check process a bit more burdensome once a PR is open though. We still go back and forth on this pretty regularly.

migrate to uv? This may be controversial - happy to drop it. Reasons to do so would be a) faster install time b) locked dependencies (easier to manage and don't break randomly) c) easier to upgrade dependencies (uv lock --upgrade). Reasons not to a) Some devs may prefer/need conda b) I'm not familiar enough with the package to see other flaws.

I've been messing around with uv and pixi on other projects this Summer, and have enjoyed the experience. We're currently migrating our build system to (likely) scikit-build-core + cibuildwheel to support upcoming compiled extensions, eg #3856. I doubt we'll change any of the current packaging/build/CI setup until after, or during only if necessary/digestibly doable, those changes come in. I don't oppose the discussion though!

Remove setup.cfg, move to using pyproject.toml fully.
Migrate fully to ruff (I know this may not be fully possible now due to #3866)

Only doable if/when we stop relying on flake8 for specific community plugins and/or our own small plugin for internal unit quantity constructors. The former has come up recently in discussion. The latter hasn't been discussed in some time I don't think.

parallelize tests (xdist?)

I haven't looked into anything like this, but am interested and curious!

@bcmyguest
Copy link
Author

@dcamron Thanks for taking a look and for the thorough reply :) Trying with 1.9.2..

linting hooks

Didn't think about this (mostly working with long running contributors) but first time PR authors is honestly a good point. Also you have linting runs anyways on CI. Something I've seen other projects do (again take it or leave it, just sharing) is have CI run the fixes itself and commit automatically. This always makes me a bit uneasy though..

uv/packages/deps

Completely fair, this is a complex project I know almost literally nothing about :S

parallel tests

We've had a good experience with pytest-xdist, but if you have compute heavy tests (I would imagine you do) it could be more harm than good. Might be worth playing around with before committing one way or the other. I might play around in the background to see what I find.

@bcmyguest
Copy link
Author

BTW not sure what you want to do with the failing code coverage. It's failing due to ruff check --fix updating a bunch of unrelated files.

broken import from pint causing broken build: hgrecco/pint#1618
@bcmyguest
Copy link
Author

matplotlib/pytest-mpl#206 (comment) looks like you know about this issue?

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Thanks for contributing the cleanup! Just one minor issue where the wrong side of the if got kept.

Let me know if you're not in a position to update and I can push the changes and merge.

Comment on lines +374 to +376
def fmt(s):
"""Perform default formatting with no decimal places and no negative 0."""
return format(round(s, 0) + 0., '.0f')
Copy link
Member

Choose a reason for hiding this comment

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

Actually, instead the format string should have been kept (apparently this got swallowed when I reviewed earlier):

Suggested change
def fmt(s):
"""Perform default formatting with no decimal places and no negative 0."""
return format(round(s, 0) + 0., '.0f')
fmt = 'z.0f'

@@ -1 +1 @@
3.10
Copy link
Member

Choose a reason for hiding this comment

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

With a dependabot bug fixed, this whole file can be deleted now.

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.

Drop Python 3.10

4 participants