-
Notifications
You must be signed in to change notification settings - Fork 438
chore: #3847 Update min python version to 3.11 #3901
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
base: main
Are you sure you want to change the base?
Conversation
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). |
- pyproj 3.6.1 added py 3.12 wheels: https://github.com/pyproj4/pyproj/releases/tag/3.6.1
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!
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.
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!
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.
I haven't looked into anything like this, but am interested and curious! |
@dcamron Thanks for taking a look and for the thorough reply :) Trying with 1.9.2..
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..
Completely fair, this is a complex project I know almost literally nothing about :S
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. |
BTW not sure what you want to do with the failing code coverage. It's failing due to |
broken import from pint causing broken build: hgrecco/pint#1618
matplotlib/pytest-mpl#206 (comment) looks like you know about this issue? |
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.
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.
def fmt(s): | ||
"""Perform default formatting with no decimal places and no negative 0.""" | ||
return format(round(s, 0) + 0., '.0f') |
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.
Actually, instead the format string should have been kept (apparently this got swallowed when I reviewed earlier):
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 |
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.
With a dependabot bug fixed, this whole file can be deleted now.
Description Of Changes
Closes #3847
Not relevant to this MR, but may be nice to have in future (and I would be happy to add if there's interest):
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.setup.cfg
, move to usingpyproject.toml
fully.ruff
(I know this may not be fully possible now due toflake8-continuation
warnings & maintenance #3866)Hope this is not overstepping, feel free to discard this PR. Have a nice day!
Checklist