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

Python 3.13 preparation #2201

Open
mikofski opened this issue Sep 7, 2024 · 22 comments · May be fixed by #2258
Open

Python 3.13 preparation #2201

mikofski opened this issue Sep 7, 2024 · 22 comments · May be fixed by #2258
Labels

Comments

@mikofski
Copy link
Member

mikofski commented Sep 7, 2024

Is your feature request related to a problem? Please describe.
Final release candidate of Python 3.13 announced. Requests upstream for prep

Describe the solution you'd like
As Python 3.13 to test matrix similar to RdTools

Describe alternatives you've considered
Put upper bound on Python compatibility

Additional context
See Google group discussion on py3.13rc02.

@kandersolar
Copy link
Member

@RDaxini or @IoannisSifnaios, any interest in continuous integration (CI) infrastructure management? Adding python 3.13 to the test matrix would be a good way to dip your toes into that world :)

Note that we can't start testing on 3.13 just yet since some of pvlib's dependencies (e.g. h5py) haven't released wheels for it yet. So Step 0 here is just to check in on the dependencies every once in a while. Once those wheels are available, we'll modify the GitHub Actions workflow files to include 3.13. See #1886 for an example of what that looks like.

@RDaxini
Copy link
Contributor

RDaxini commented Sep 9, 2024

@kandersolar Sure I am interested.

I was reviewing #1886 as advised and I noticed here that although some of the dependencies were unavailable in python 3.12, e.g. numba, this does not seem to have been a problem(?) ---why not?

This is not something with which I have any experience so I might just be missing/misunderstanding something obvious 😅

@kandersolar
Copy link
Member

It's a good question! The reason is that some of pvlib's dependencies are required while others are merely optional. "Required" means that pip install pvlib will fail if pip can't locate a suitable installation file for the dependency. "Optional" ones are not required for installation to succeed, but of course some pvlib functionality is not available unless the relevant dependency is installed. Our tests are configured such that tests for that functionality get skipped if the relevant optional dependency isn't installed.

You can check that the dependencies omitted in #1886 are all optional dependencies as defined in pyproject.toml:

dependencies = [
'numpy >= 1.17.3',
'pandas >= 1.3.0',
'pytz',
'requests',
'scipy >= 1.6.0',
'h5py',
]

[project.optional-dependencies]
optional = [
'cython',
'ephem',
'nrel-pysam',
'numba >= 0.17.0',
'solarfactors',
'statsmodels',
]

And just to finish the story, we did add those dependencies back in once wheels became available: #1964

@IoannisSifnaios
Copy link
Contributor

@kandersolar is there an easy way of finding which optional dependencies do not work with python 3.13? E.g., if we make a PR, would we get an error about which ones are not compatible? Or do we have to manually check each one of them to see if it is 3.13 compatible or not?

@kandersolar
Copy link
Member

Yes a PR would certainly tell you whether the package landscape is sufficiently progressed for us to start using it for pvlib. I suppose there is no harm in opening a PR now, with the expectation that it will remain open for some time waiting for the dependencies to become available. In fact the current roadblock may be whether python 3.13 itself is installable on the CI yet :P

@echedey-ls
Copy link
Contributor

We need to bring back the discussion of solarfactors again pvlib/solarfactors#16
There will certainly be problems with all the code that relies on solarfactors.

@IoannisSifnaios
Copy link
Contributor

Yes a PR would certainly tell you whether the package landscape is sufficiently progressed for us to start using it for pvlib. I suppose there is no harm in opening a PR now, with the expectation that it will remain open for some time waiting for the dependencies to become available. In fact the current roadblock may be whether python 3.13 itself is installable on the CI yet :P

Makes sense! I guess we should at least wait for the official release (1/10/2024)

@AdamRJensen AdamRJensen linked a pull request Oct 11, 2024 that will close this issue
8 tasks
@markcampanelli
Copy link
Contributor

I will ask again: Please start putting upper bounds on the Python versions supported in the pyproject.toml.

python -m pip install -e .[all] is simply broken ATM on Python 3.13.0.

If I update the pyproject.toml to clearly state requires-python = ">=3.9, <3.13", then I get a very clear and actionable error message:

ERROR: Package 'pvlib' requires a different Python: 3.13.0 not in '<3.13,>=3.9'

@markcampanelli
Copy link
Contributor

markcampanelli commented Dec 21, 2024

A more granular way to handle this so that optional dependencies don't hold users back is:

[project.optional-dependencies]
optional = [
    'cython',
    'ephem',
    'nrel-pysam;python_version<"3.13"',
    'numba >= 0.17.0;python_version<"3.13"',
    'solarfactors;python_version<"3.12"',
    'statsmodels',
]

Note that solarfactors also shows up in the doc section , and I don't know if the upper exclusion limits here may actually be lower than 3.13 .

All that said, please still use requires-python = ">=3.9, <3.13" until pvlib is actually being built and tested in CI on Python 3.13.

UPDATED: solarfactors apparently requires Python <3.12, which I updated here.

@echedey-ls
Copy link
Contributor

@markcampanelli are those all the dependencies that would need to get pinned? I understand the problem, I see how this mitigates the situation, but I think the overall best solution would be that our dependencies make sure they work for the latest pythons - or at least pin the maximum version they work with.

I don't really like the idea of somebody testing periodically whether or not pvlib deps are finally compatible, and do a patch release just for that. Btw, for anyone still not aware of the solarfactors problem, it will persist there until somebody rewrites it from scratch (or somehow updates all the code affected; spoilers, it's pretty bad).

Btw, at least in PySAM they are unaware of the problem - 3.13 not being tested nor an issue is opened.

@markcampanelli
Copy link
Contributor

@echedey-ls See #2339 for how I would revise the current pyproject.toml. I note that nowhere in the current pvlib documentation could I find what versions of Python are supported. I would still add to this PR a section in the documentation somewhere that records this AND the dependency-management policy, for which I would propose something along the lines of:

  1. requires-python always has a lower and upper bound that corresponds to all the minor versions of Python 3 that are tested in CI.
  2. All non-optional dependencies always have a lower and upper bound. (I have not done this with pytz in the above PR, but it looks like as of Python 3.9 it can be replaced by the standard library. I may try to do this in a separate PR :).
  3. Optional dependencies do not have to be bounded above or below, but they may be restricted in their Python version support in order for users to use the latest Python (once it is supported by the range in requires-python).

Personally, I would ALSO put lower and upper bounds on everything the optional, doc, and test sections, but that's because I've been burned way too many times by my CI breaking "out of the blue" because, say, pytest had a breaking major version bump. Also, any pre-1.0 package I tend to limit to only patches, e.g., sphinx-toggleprompt >=0.5.2, <0.6.

This doesn't prevent all possible ways that things can go sideways with dependencies (which I'm convinced is nearly infinite!), but usually there has to be a bug or someone else has to screw up semver for this approach to fail. Maybe all this seems pedantic and creating extra maintenance work, but I would say PLEASE resist the belief that one can predict the future with respect to dependencies. Also, this type of policy and the required maintenance will encourage a minimal dependency tree, which I view as generally a good practice that brings much developer sanity in the long run.

@mikofski
Copy link
Member Author

mikofski commented Dec 22, 2024

This seems related to CFEP-25

A frequent issue observed with noarch: python packages has been lower bounds that are not being updated when the upstream package drops support for a Python version. As a result, the noarch: python package continues to be built and installed for Python versions the package no longer officially supports. Sometimes users get lucky and no issues occur. Other times pulling in the package breaks an older Python install they are using. The result is work by users, maintainers, and core to fix the package metadata and patch the repodata. Eating into time that might be better spent by all on other issues.

See: conda-forge/pvlib-python-feedstock#46

@markcampanelli
Copy link
Contributor

@echedey-ls I apologize that I didn't actually answer your main question above. Yes, I think those are all the optional dependencies that would need to be limited w.r.t. the Python version. I was able to pip-install [all] using this pyproject.toml in Python 3.10, 3.11, and 3.12 (and in 3.13, for that matter, after removing the requires-python upper limit) on x86 Ubuntu 18.0.4.6 LTS. The real test is of course the CI builds across several architectures and such, however, and I did not look into the CI logic that is already to manage the known optional dependency breakages. (Also, I have no idea about implications for conda.)

@kandersolar
Copy link
Member

The problem is that current and previous versions of pvlib are in fact compatible with Python 3.13. That fact does not depend on whether PyPI makes it easy to install pvlib's (optional?) dependencies on Python 3.13. I don't think imposing a python<3.13 requirement on pvlib is justified by the lag time of the broader python ecosystem catching up with new python releases.

I remain of the view that the arguments in favor of upper bounds apply to applications, not libraries, and pvlib is a library. It is one piece of a puzzle. Where pvlib is being used are part of an application, the onus of ensuring a working environment for that application is on the application's developer.

I note that nowhere in the current pvlib documentation could I find what versions of Python are supported.

The docs Installation page used to say e.g. "pvlib is compatible with python 3.5 and above", but it was a conscious decision to remove that (#1035). I think that could be reconsidered, but note that the PyPI page automatically reports the python requirements based on pyproject.toml. Of course the whatsnew pages also list every time we drop support for a version.

@markcampanelli
Copy link
Contributor

@kandersolar

The problem is that current and previous versions of pvlib are in fact compatible with Python 3.13.

Putting my software engineer/QA hat on for a moment: The fact that pvlib’s CI does not currently run build and test on Python 3.13 on the main branch means that I (pretending, say, to be an agnostic outsider using pvlib as a library in “production”) do not trust this claim enough to use pvlib with Python 3.13 for any work that is not experimental.

All I am requesting is that the upper Python bound be increased in sync with build and test being proven in CI on the main branch. I am totally ok if the optional dependencies retain (and declare, for much improved clarity) lower upper bounds as needed.

@markcampanelli
Copy link
Contributor

@kandersolar

My apologies if I am coming off as insensitive here to the additional maintenance burden of my suggestion. I think Python now releases minor bumps every six months, and major breaks in scipy, numpy, pandas, etc. are much more rare than that. So, at least every six months someone would have to run through a dependency check (presumably at the same time that they add the next Python version to the CI test matrix and possibly remove older versions). I totally appreciate that this eats up time that we’d all rather spend on, say, delivering a new algorithm.

I am willing to accept that that maintenance burden is too much ATM, in which case let’s just be more explicit about this policy in the docs so users cannot say they weren’t warned.

@kandersolar
Copy link
Member

kandersolar commented Dec 23, 2024

For once, my hesitation doesn't stem from any additional maintenance burden :P

My understanding is that putting an upper bound on Python itself is not actually a solution to any problem. Regardless of whether or not it's pvlib's responsibility to provide such a bound (which is a philosophical disagreement), there is a very real-world issue here: putting an upper bound in requires-python prevents the current version of pvlib from being installed not by raising an error message but rather by installing an older version of pvlib instead.

Say that we released a v0.11.3 with requires-python:>=3.9,<3.13 and then created a Python 3.13 environment and ran pip install pvlib. Here's what would happen:

  1. pip would examine pvlib==0.11.3 and see that it it isn't compatible with Python 3.13.
  2. pip would backsolve to look for some version of pvlib that is marked as compatible with Python 3.13.
  3. pip would find pvlib==0.11.2, which has no upper bound in Python-Requires.
  4. pip would install pvlib==0.11.2 without complaint.

Of course this doesn't actually solve the problem at hand; arguably it just makes things worse. As far as I know, this issue makes upper bounds in requires-python basically a non-starter. For details, see this discussion by central members of the python packaging community (i.e., people that understand things much better than I do): https://discuss.python.org/t/requires-python-upper-limits/12663/4

I suppose the reason that you got a nice error message (ERROR: Package 'pvlib' requires a different Python: 3.13.0 not in '<3.13,>=3.9') is because you were installing the package locally, where there are no alternative versions to consider. I don't think we would get that nice error message if we deployed that change to a new version on PyPI.

@markcampanelli
Copy link
Contributor

@kandersolar I think I see your point now, thanks. However, it sounds to me a bit like "It's been broken since the beginning, so don't fix it now." Seems like a user can stumble into untested code either way. If we were to fix this going forward, then the user would have to realize that they didn't get the latest, and perhaps do something like

python -m pip install pvlib==0.11.3

at which point they would get the error if their Python were out of range. I think I still prefer this, but I'll noodle on if there is a more elegant way to move forward with the change.

@kandersolar
Copy link
Member

However, it sounds to me a bit like "It's been broken since the beginning, so don't fix it now."

Just to be clear: my understanding is that this should say can't fix, not don't fix. requires-python is not meant for upper bounds, and the tooling does not use it in a way that achieves the stated goal here. This is not a pvlib problem, it's a python packaging problem.

@echedey-ls
Copy link
Contributor

I don't want to distract from the main point of this issue, but would it help if somehow we show in the README (by extension, in PyPI) or in trove classifiers which python versions are currently tested against? I hate proposing such an indirect mitigation, but may help at announcing the python versions pvlib and it's dependencies can run on.

@markcampanelli
Copy link
Contributor

@echedey-ls One can specify the minor version of Python in the classifiers section of the pyproject.toml, which I do in pvfit, and which shows up fairly prominently in the "Programming Language" sidebar on PyPI:

classifiers =[
    "Development Status :: 3 - Alpha",
    "Intended Audience :: Developers",
    "Intended Audience :: Education",
    "Intended Audience :: End Users/Desktop",
    "Intended Audience :: Science/Research",
    "Natural Language :: English",
    "Topic :: Scientific/Engineering",
    "Topic :: Software Development :: Libraries",
    "License :: OSI Approved :: MIT License",
    "Programming Language :: Python :: 3",
    "Programming Language :: Python :: 3.10",
    "Programming Language :: Python :: 3.11",
    "Programming Language :: Python :: 3.12",
]

This reminds me: I still need to add 3.13 to my test matrix, update my python_requires to ">=3.10, <3.14", and release a new pvfit tag that supports Python 3.13 😉.

@echedey-ls
Copy link
Contributor

Thanks for reminding that @markcampanelli ; my point is simpler than that. If we show the currently CI tested Python versions in PyPI or in the readme with a badge similar to FastAPI's, would that make it easier for you to know when the next Python version is already tested? (Instead of you having to test locally)

image

https://github.com/fastapi/fastapi/blob/1d50bad4555b951d5fa48eb8787fd5c39ed7effb/README.md?plain=1#L17C1-L19C5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants