-
Notifications
You must be signed in to change notification settings - Fork 24
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
Increment Python > 3.8
#285
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #285 +/- ##
===========================================
- Coverage 95.48% 95.47% -0.01%
===========================================
Files 36 36
Lines 2036 2034 -2
===========================================
- Hits 1944 1942 -2
Misses 92 92 ☔ View full report in Codecov by Sentry. |
@@ -9,7 +9,7 @@ | |||
|
|||
# To update the matrix, the variables below can be modified as needed. | |||
|
|||
python_version=("3.8" "3.9" "3.10" "3.11" "3.12") | |||
python_version=("3.9" "3.10" "3.11" "3.12") |
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.
This comment is not a review, but more of a question about our policy for dropping these versions. Is the plan to follow PyBaMM strictly and create another release for PyBOP as soon as possible after PyBaMM's own release?
There could be a case where PyBaMM drops support for Python v3.A in its new version v2X.Y, and a current PyBOP v2M.N might still support Python v3.A, which means one matrix target will start to fail, creating a signal to drop the version for us here at PyBOP. Users running PyBOP v2M.N at that time on Python v3.A will notice that the new PyBaMM v2X.Y will not be downloaded by pip
because of the Python version constraint, and therefore they would be using an older version, say PyBaMM v2X.W, which is alright. However, those on newer versions of Python, say, v3.B, will not face this constraint because it will be supported by both PyBOP and PyBaMM (assuming that both packages will support an array of at least four Python versions simultaneously). Therefore v3.B will compel pip
to download PyBaMM v2X.Y + PyBOP v2M.N. Here, there could be breaking changes for PyBaMM which PyBOP would not be supporting. As a result, PyBOP would need to put out a new release after v2M.N with a new identifier, say, v2M.O to support the new PyBaMM v2X.Y along with previous PyBaMM versions (but if it's not possible to support older ones, then drop an old one and support a new one at the same time) and/or drop support for Python v3.A (and add support for newer Python versions supported by PyBaMM). Therefore, I have these comments:
- Maybe PyBOP should consider upper-pinning PyBaMM in its dependencies (in addition to lower bounds) to avoid such behaviour
a. There is a lot of nuance involved with upper-bound constraints: https://iscinumpy.dev/post/bound-version-constraints/ - The answer is also highly dependent on how well PyBaMM documents its breaking changes and features, and also on the extent of the breaking changes (it's unlikely that, say, a PyBaMM solver changes its functionality significantly or anything, but other things like parameter sets with the BPX standardisation could do so – that I do not have a lot of information about). It would also be reliant on how users are going to use PyBaMM and PyBOP together and how reliant the latter will be on the former in terms of certain functionality and offering convenient features.
- I suppose these issues would only be encountered on those that are running edge versions of Python (say, Python v3.A and Python v3.D), and not those intermediate ones (Python v3.B, v3.C, etc.) that will continue to stay in support for years on stretch and will be dropped only after a considerable amount of releases (i.e., not in the immediate next releases for either of PyBaMM or PyBOP).
I guess I am just concerned about the synchronicity between PyBaMM and PyBOP; there is a chance that my concerns could be misplaced, but I hope that what I am asking makes a bit of sense and is something to ponder about!
P.S. Adjusting this script to cater to new Python and PyBaMM versions or ignore specific ones is quite trivial, of course, and we don't need to worry about it.
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.
P.P.S. the answer would also be dependent upon how intersecting the user base of both packages is going to be (which I assume it will intersect by quite a lot), and on how much the PyBOP core developers will be following up on changes to/in PyBaMM, so I guess there is no right answer here and everything will be evaluated on a case-by-case basis (the longer release cycle between both packages helps I guess!). As someone who does not use both packages that frequently and doesn't do battery modelling as a part of any institutional positions or an industrial job, I might be ringing the wrong bell here, so please feel free to redirect me.
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.
Hi Agriya,
Great question, and one I've been thinking about. I think we should drop Python versions to follow PyBaMM (which then follows NumPy) within a release. We should definitely upper-pin PyBaMM, as I've noticed that there are still enough breaking changes from release to release to cause problems for PyBOP. I think this will mostly solve any Python version conflicts.
I've also been thinking about how we support the range of PyBaMM versions, it will be challenging as we add/request functionality to PyBaMM to support PyBOP methods. For example, #252 was introduced because PyBaMM v23.5 doesn't have the Thevenin variables we need.
I think an update to the README is in order to add more granularity for users using PyBaMM standalone as well as PyBOP. Most of this should be solvable with proper virtualenvs, but I'm sure there will be edge cases and it will be good to have documentation to point users to.
Thanks for the review and the discussion!
Co-authored-by: Agriya Khetarpal <[email protected]>
Description
Increments Python version to be > 3.8.
Issue reference
Fixes #284
Review
Before you mark your PR as ready for review, please ensure that you've considered the following:
Type of change
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ nox -s tests
$ nox -s doctest
You can run integration tests, unit tests, and doctests together at once, using
$ nox -s quick
.Further checks:
Thank you for contributing to our project! Your efforts help us to deliver great software.