pyhf icon indicating copy to clipboard operation
pyhf copied to clipboard

chore: Drop Python 3.8, fix issues with CI

Open kratsg opened this issue 1 year ago • 2 comments

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.com

$ curl -A "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:131.0) Gecko/20100101 Firefox/131.0" --head https://academic.oup.com/ptep/article/doi/10.1093/ptep/ptad144/7459362
HTTP/2 403
date: Sat, 21 Dec 2024 15:59:02 GMT
content-type: text/html; charset=UTF-8
content-length: 8743
accept-ch: Sec-CH-UA-Bitness, Sec-CH-UA-Arch, Sec-CH-UA-Full-Version, Sec-CH-UA-Mobile, Sec-CH-UA-Model, Sec-CH-UA-Platform-Version, Sec-CH-UA-Full-Version-List, Sec-CH-UA-Platform, Sec-CH-UA, UA-Bitness, UA-Arch, UA-Full-Version, UA-Mobile, UA-Model, UA-Platform-Version, UA-Platform, UA
critical-ch: Sec-CH-UA-Bitness, Sec-CH-UA-Arch, Sec-CH-UA-Full-Version, Sec-CH-UA-Mobile, Sec-CH-UA-Model, Sec-CH-UA-Platform-Version, Sec-CH-UA-Full-Version-List, Sec-CH-UA-Platform, Sec-CH-UA, UA-Bitness, UA-Arch, UA-Full-Version, UA-Mobile, UA-Model, UA-Platform-Version, UA-Platform, UA
cross-origin-embedder-policy: require-corp
cross-origin-opener-policy: same-origin
cross-origin-resource-policy: same-origin
origin-agent-cluster: ?1
permissions-policy: accelerometer=(),autoplay=(),browsing-topics=(),camera=(),clipboard-read=(),clipboard-write=(),geolocation=(),gyroscope=(),hid=(),interest-cohort=(),magnetometer=(),microphone=(),payment=(),publickey-credentials-get=(),screen-wake-lock=(),serial=(),sync-xhr=(),usb=()
referrer-policy: same-origin
x-content-options: nosniff
x-frame-options: SAMEORIGIN
cf-mitigated: challenge
cf-chl-out: kI3rohlrq71KKbX3Nu5hEk+BZm/glZYADDHnftN2mhoifmycVKBIEKZYvPCsVb+xsTEwla38Dd5bmTzO5Zp1DqjaKNHtcAHESRv5Df0hp5cFOzHELElOSA4pTk2t2zdF8wKDrblHVBoC2DQeg2+wvw==$NSFsXK2alPB8IfU0cnzHYA==
cache-control: private, max-age=0, no-store, no-cache, must-revalidate, post-check=0, pre-check=0
expires: Thu, 01 Jan 1970 00:00:01 GMT
set-cookie: __cf_bm=exWKl5QVT3Iw5iQaVpCxbexRIUQTkPiI1lk1izdzRtQ-1734796742-1.0.1.1-BwvBPQzFIvTKf0JO9umxyVZjkbA1Yoi6Ci1uTiHurJ24dBv2dcohoqmsUBkRU3SC_qHJQ5euBC9fT5p4G4HBDg; path=/; expires=Sat, 21-Dec-24 16:29:02 GMT; domain=.academic.oup.com; HttpOnly; Secure; SameSite=None
server: cloudflare
cf-ray: 8f5913baf8e48da6-MIA

for journals.aps.org

$ curl -A "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:131.0) Gecko/20100101 Firefox/131.0" --head https://journals.aps.org/prd/abstract/10.1103/PhysRevD.106.032005
HTTP/2 403
date: Sat, 21 Dec 2024 16:02:20 GMT
content-type: text/html; charset=UTF-8
content-length: 8677
accept-ch: Sec-CH-UA-Bitness, Sec-CH-UA-Arch, Sec-CH-UA-Full-Version, Sec-CH-UA-Mobile, Sec-CH-UA-Model, Sec-CH-UA-Platform-Version, Sec-CH-UA-Full-Version-List, Sec-CH-UA-Platform, Sec-CH-UA, UA-Bitness, UA-Arch, UA-Full-Version, UA-Mobile, UA-Model, UA-Platform-Version, UA-Platform, UA
critical-ch: Sec-CH-UA-Bitness, Sec-CH-UA-Arch, Sec-CH-UA-Full-Version, Sec-CH-UA-Mobile, Sec-CH-UA-Model, Sec-CH-UA-Platform-Version, Sec-CH-UA-Full-Version-List, Sec-CH-UA-Platform, Sec-CH-UA, UA-Bitness, UA-Arch, UA-Full-Version, UA-Mobile, UA-Model, UA-Platform-Version, UA-Platform, UA
cross-origin-embedder-policy: require-corp
cross-origin-opener-policy: same-origin
cross-origin-resource-policy: same-origin
origin-agent-cluster: ?1
permissions-policy: accelerometer=(),autoplay=(),browsing-topics=(),camera=(),clipboard-read=(),clipboard-write=(),geolocation=(),gyroscope=(),hid=(),interest-cohort=(),magnetometer=(),microphone=(),payment=(),publickey-credentials-get=(),screen-wake-lock=(),serial=(),sync-xhr=(),usb=()
referrer-policy: same-origin
x-content-options: nosniff
x-frame-options: SAMEORIGIN
cf-mitigated: challenge
cf-chl-out: 1GM7L6OnEIhUA3Nu78SsSXsSZPAKkHusPvR9rzOM+i94tqt6nJbC6KqH+WQcDpnX8c7VY59qsL5H7k14HA/7XtJvoWjCKBm6mKwjXcEfoNtFIA/B+0lWpJumsFsbjBrRK2PmdOH3Uyb6DMRmZXWYCw==$ndOh1e20Lbf2J+XAsQTYgQ==
cache-control: private, max-age=0, no-store, no-cache, must-revalidate, post-check=0, pre-check=0
expires: Thu, 01 Jan 1970 00:00:01 GMT
server: cloudflare
cf-ray: 8f59188e4f4d7469-MIA
alt-svc: h3=":443"; ma=86400

Checklist Before Requesting Reviewer

  • [x] Tests are passing
  • [x] "WIP" removed from the title of the pull request
  • [x] Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • [x] Summarize commit messages into a comprehensive review of the PR
* drop python 3.8 support everywhere
* fix failing CI on main branch by moving from python 3.9 to python 3.10 for the mypy minimum python check, due to numpy typing being improved in 3.10+
* disable linkchecks for aps.org, academic.oup.com as they 403 all HEAD requests
* cast modifier masks to the default_backend's `bool` array initially to resolve pytorch warnings
* migrate `get_backend` to resolve import issues seen in the CI
* fix doctest failures on macos-13 compared to ubuntu-latest due to differing numpy major versions being picked up in the CI (ubuntu gets 2.x while macos is getting 1.x) which has different `repr` for some common functions

kratsg avatar Dec 21 '24 14:12 kratsg

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 98.23%. Comparing base (157fc95) to head (ebfaad9). :warning: Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2566   +/-   ##
=======================================
  Coverage   98.23%   98.23%           
=======================================
  Files          65       65           
  Lines        4190     4190           
  Branches      452      452           
=======================================
  Hits         4116     4116           
  Misses         45       45           
  Partials       29       29           
Flag Coverage Δ
contrib 98.11% <ø> (ø)
doctest 98.23% <ø> (ø)
unittests-3.10 96.42% <ø> (ø)
unittests-3.11 96.42% <ø> (ø)
unittests-3.12 96.42% <ø> (ø)
unittests-3.13 96.42% <ø> (ø)
unittests-3.9 96.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Dec 21 '24 17:12 codecov[bot]

ping @matthewfeickert

kratsg avatar Mar 24 '25 13:03 kratsg

The other thing that I wanted to get your 👍 / 👎 on is that this is a pretty dang big PR (for good reason!). As we know that there's going to be some CI failures that we have to fix anyway, how do you feel about pulling all of the "drop TensorFlow" stuff out into a seperate PR so that this gets more atomic? Given that you've done all the work for this PR by yourself, I'm happy to volunteer to do that PR so that I don't give you Git surgery.

It's impossible. We've pushed this PR so far back that we have to drop pytorch AND tensorflow AND have these fixes just to have a working CI. I cannot tell you what to fix if you just drop one dependency without fixing the existing CI. You're stuck with this big PR I think.

kratsg avatar Sep 30 '25 20:09 kratsg

It's impossible. We've pushed this PR so far back that we have to drop pytorch AND tensorflow AND have these fixes just to have a working CI. I cannot tell you what to fix if you just drop one dependency without fixing the existing CI. You're stuck with this big PR I think.

I agree it isn't possible to get the CI passing without a combination of things. But as we already have failing CI in some places I don't think it is a bad thing to say "we understand this" but let's break our fixes up into more focused PRs (which importantly will make cherry-picking things easier if needed).

matthewfeickert avatar Sep 30 '25 21:09 matthewfeickert

This is currently reverted atm(?), but for the

          - os: macos-15-intel
            python-version: '3.13'

change to CI if we know that x86 macOS isn't going to be supported by most of our backends for Python 3.13, should we just test it at Python 3.12 or whatever the highest version where there are still wheels? That would make it pretty clear when we'll have to drop x86 macOS support, as the decision will have been made for us.

matthewfeickert avatar Sep 30 '25 21:09 matthewfeickert

PR https://github.com/scikit-hep/pyhf/pull/2609 (tests are passing) is an example of what I mean RE: factoring out removal of backends into more atomic PRs.

matthewfeickert avatar Oct 01 '25 00:10 matthewfeickert

@kratsg I think if you rebase this then it should be ready to go.

matthewfeickert avatar Oct 07 '25 11:10 matthewfeickert

@kratsg Why are these additional import changes needed? PR https://github.com/scikit-hep/pyhf/pull/2625 is a subset of this (involves a cherry-pick+squash) and it doesn't require those changes.

matthewfeickert avatar Nov 05 '25 17:11 matthewfeickert

@kratsg Why are these additional import changes needed? PR #2625 is a subset of this (involves a cherry-pick+squash) and it doesn't require those changes.

because you didn't bump the minimum mypy version for python to 3.10. This changes enough in python typing (we shouldn't use 3.9 for typing).

kratsg avatar Nov 05 '25 18:11 kratsg

because you didn't bump the minimum mypy version for python to 3.10. This changes enough in python typing (we shouldn't use 3.9 for typing).

@kratsg I did though: https://github.com/scikit-hep/pyhf/pull/2625/files#r2495650423

From my fork's branch

$ git diff upstream/main  origin/build/drop-python-3-8-support -- .pre-commit-config.yaml
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 07539686..ef188fe0 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -55,14 +55,15 @@ repos:
 -   repo: https://github.com/pre-commit/mirrors-mypy
     rev: v1.13.0
     # check the oldest and newest supported Pythons
+    # except skip python 3.9 for numpy, due to poor typing
     hooks:
       - &mypy
         id: mypy
-        name: mypy with Python 3.8
+        name: mypy with Python 3.10
         files: src
         additional_dependencies:
           ['numpy', 'types-tqdm', 'click', 'types-jsonpatch', 'types-pyyaml', 'types-jsonschema', 'importlib_metadata', 'packaging']
-        args: ["--python-version=3.8"]
+        args: ["--python-version=3.10"]
       - <<: *mypy
         name: mypy with Python 3.13
         args: ["--python-version=3.13"]

or just comparing to this PR's branch

$ git diff upstream/fix/ci origin/build/drop-python-3-8-support -- .pre-commit-config.yaml
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index b22720ce..ef188fe0 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -65,8 +65,8 @@ repos:
           ['numpy', 'types-tqdm', 'click', 'types-jsonpatch', 'types-pyyaml', 'types-jsonschema', 'importlib_metadata', 'packaging']
         args: ["--python-version=3.10"]
       - <<: *mypy
-        name: mypy with Python 3.12
-        args: ["--python-version=3.12"]
+        name: mypy with Python 3.13
+        args: ["--python-version=3.13"]
 
 -   repo: https://github.com/codespell-project/codespell
     rev: v2.3.0

matthewfeickert avatar Nov 05 '25 18:11 matthewfeickert