chore: Drop Python 3.8, fix issues with CI #2566
Conversation
kratsg
added
feat/enhancement
kratsg
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2566 +/- ##
==========================================
+ Coverage 98.21% 98.24% +0.02%
==========================================
Files 69 69
Lines 4543 4546 +3
Branches 804 467 -337
==========================================
+ Hits 4462 4466 +4
Misses 48 48
+ Partials 33 32 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
matthewfeickert
force-pushed
the
fix/ci
branch
from
0f28a49 to
42a5249
Compare
| @@ -120,7 +120,7 @@ def cdf(self, value): | |||
| >>> import pyhf | |||
| >>> pyhf.set_backend("numpy") | |||
| >>> bkg_dist = pyhf.infer.calculators.AsymptoticTestStatDistribution(0.0) | |||
| >>> bkg_dist.cdf(0.0) | |||
| >>> print(bkg_dist.cdf(0.0)) | |||
There was a problem hiding this comment.
Did something change RE: the way doctest works where the prints are now necessary? We had always treated this like REPL behavior, not script behavior.
There was a problem hiding this comment.
No, numpy changed. ubuntu-latest has numpy 2.x which gives np.float64(0.0) while macos-13 uses numpy 1.x which gives 0.0 -- we can't write a test that relies on repr for two diff major versions of numpy.
There was a problem hiding this comment.
>>> import numpy as np
>>> np.__version__
'2.0.2'
>>> np.float64(0.0)
np.float64(0.0)
>>> print(np.float64(0.0))
0.0and
>>> import numpy as np
>>> np.__version__
'1.26.4'
>>> np.float64(0.0)
0.0
>>> print(np.float64(0.0))
0.0There was a problem hiding this comment.
we can't write a test that relies on
reprfor two diff major versions of numpy.
I understand what you mean, but I thought that we already did have ways to indicate to doc tests that it should only test on certain operating systems as we would already get different versions.
Let me take the afternoon today (once I'm free from running interviews for a search I'm on) to play with this and if I can't get it then we can just accept it and move on.
| @@ -11,7 +11,7 @@ dynamic = ["version"] | |||
| description = "pure-Python HistFactory implementation with tensors and autodiff" | |||
| readme = "README.rst" | |||
| license = { text = "Apache-2.0" } # SPDX short identifier | |||
| requires-python = ">=3.8" | |||
| requires-python = ">=3.9" | |||
There was a problem hiding this comment.
I'm personally fine with jumping up to Python 3.9. I think though as the 0.7.6 release is still on Python 3.7 that regardless of features we should make a 0.8.0 release after this that is the same as what we have in the 0.7.x series but just with non-EOL Python support. That will make multiple things easier to maintain and Python 3.9 is not new at all so I don't see that as an issue.
| @@ -101,6 +101,8 @@ class histosys_combined: | |||
| def __init__( | |||
| self, modifiers, pdfconfig, builder_data, interpcode='code0', batch_size=None | |||
| ): | |||
| default_backend = pyhf.default_backend | |||
There was a problem hiding this comment.
Is this needed just to deal with the PyTorch warnings from Issue #2554?
There was a problem hiding this comment.
it's needed in order to type _histosys_mask correctly as a tensor (see the below lines where it is being used). You could drop it, but then the typehints won't work due to the changes in typing in numpy.
Pull Request Description
Drop python 3.8 support (would be hard to fix most typing-related issues anyway). This will also fix maintenance issues with the CI we've been having hopefully.
Also linkcheck gives 403 for some URLs because they're blocking HEADs now... not sure why that changed.
for
academic.oup.comfor
journals.aps.orgChecklist Before Requesting Reviewer
Before Merging
For the PR Assignees: