-
Notifications
You must be signed in to change notification settings - Fork 354
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
add python 3.12 to the build and drop python 3.8 #4716
Conversation
Ok, I'm tracking through various incompatibilities. One issue is the installation of sbank. We need newer wheels. I've started a PR to see if this is easy on the sbank end. |
Ok, there a couple other problems here. (1) there are some places we are not actually 3.12 compliant, I am going through to fix some of these. (2) the docker / venv builds need to be moved to a later python version to match (currently broken). (3) I am seeing some build issues with some upstream libraries. These might be workable, but I'm having trouble actually getting some libraries like ligo.lw to actually build locally for testing (somehow seems to work on github??). |
@maxtrevor note that we are using 3.9 for PyCBC Live at the moment; if we decide to move to a PyCBC version that includes this change, we will probably need to rebuild the testing and production envs from scratch. |
Ok, I'll revert dropping python 3.9. |
SBank python 3.12 wheels are available now. |
Ok, I think for this to make the most sense we need to |
Note, at the moment, docker / venv are kept at python 3.9, but this seems to work and notably numpy is making releases for python3.9 with numpy 2.0. I don't expect this to last, so it would be good to advance these versions at some point, but I think we can postpone dropping 3.9 if needed. |
@spxiwh Can you please review? There is one blocking issue now, which is that the tutorials need to be updated to work and that is a blocking issue for merging, however, I don't think that should involve changes to pycbc unless we want to keep monkey patching emcee 2 to work. I think it's the right time to drop though for future python versions. @cdcapano and I will need to address But it would be good to get any feedback on the remainder of the PR or if we are otherwise good to go once the tutorial tests pass (which again should just be a rerun of the CI once we address the tutorials themselves). |
#4821 seems related to this. |
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 all looks reasonably clear, and sensible. I left some minor comments, but am happy to approve once tests work.
@@ -13,7 +13,7 @@ jobs: | |||
max-parallel: 60 | |||
matrix: | |||
os: [ubuntu-20.04] | |||
python-version: [3.8, 3.9, '3.10', '3.11'] |
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.
Why are we dropping 3.9 from the basic tests as well?
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.
I'll add this back. 3.9 is past EOL, but we can remove later after the docker / venv images are also updated and pycbc live is moved to a newer python version. I believe those are the hangups for 3.9.
import importlib.util | ||
import importlib.machinery | ||
|
||
def load_source(modname, filename): |
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.
Can we move this function to some appropriate module, rather than defining in executables?
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 is duplicated in a few places, and this avoids that ...
"numpy==1.23.4; python_version=='3.11'", | ||
"numpy; python_version >= '3.12'", | ||
"cython>=0.29.21", | ||
"numpy>=2.0.0", |
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.
Should we change PR title to indicate that this now makes pycbc require numpy 2 as well as dropping python versions?
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.
Is it actually necessary to require Numpy 2? As far as I understood it was supposed to maintain some level of backward compatibility.
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.
We don't require numpy 2.0. However, to work with 2.0 it must be built with 2.0. They have added in 2.0 the ability for ABI to be backwards compatible, so when it is built with 2.0 it will still work with 1.XX.
See
https://numpy.org/devdocs/dev/depending_on_numpy.html#numpy-2-0-specific-advice
That's why i did not update the setup.py or requirements file at hte same time, only the pyproject.toml which dictates how the build itself is done.
@@ -1,4 +1,5 @@ | |||
# For LDG service access | |||
ligo-proxy-utils |
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.
What is this needed for?
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.
@spxiwh That's a question for you all actually. If it's not needed we can remove it. However, it was hard coded in the workflow whereas if it is needed it should be in the requirements rather than installed by hand.
* drop python3.7, 3.8, 3.9 * add missing quotes * banish distutils * banish p39 in more places * tox needs some things early * move here * maybe? * try el9 * must use 311 for el9 * this might be the right name * updates * try this * more stuff * update * do not use imp module * more places where imp was used * bring back 39 * more reverts * update * update * fixes * this should be here * cc * fix web url * add back 3.9 to basic tests * centralize python12 backwards compatibility function * cc * simplify setup.py, ensure math is linked * cc
Motivation
This moves pycbc to 3.09 as the minimum python version. This is consistent with upstream numpy release schedule which we in principle try to sync to. This will reduce the number of tests and allow people to being using some more modern python structures. Python 3.12 is also added to the test infrastructure and build.
Contents
Links to any issues or associated PRs
Testing performed
The CI will hopefully provide the needed tests. Iteration may be needed to ensure the various env builds all work now.