-
Notifications
You must be signed in to change notification settings - Fork 18
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
DM-48903: Python 3.12 and other upgrades #893
Conversation
.github/workflows/ci.yml
Outdated
dir: "build/doc/html" | ||
username: ${{ secrets.LTD_USERNAME }} | ||
password: ${{ secrets.LTD_PASSWORD }} | ||
|
||
update-run-image: | ||
name: Update Qserv image | ||
runs-on: ubuntu-20.04 |
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.
From GitHub: "The Ubuntu 20.04 runner image will be fully unsupported by April 1, 2025."
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.
Ubuntu-24.04 is LTS
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.
Thanks -- will update...
&& cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=/usr/local -DBUILD_ONLY="s3" -DUSE_TLS_V1_3=on -DCPP_STANDARD=17 \ | ||
&& cmake --build . \ | ||
&& cmake --install . \ | ||
&& make -j8 \ |
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 a fixed number? You get the actual number of CPU cores on the machine in Linux by:
$(cat /proc/cpuinfo | grep siblings | sort -u | awk '{print $3}')
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.
Hmmm. Copied from elsewhere -- the "8" was determined at some point to be a reasonable default; using all available CPUs was locking up peoples' machines during builds. We could put something more sophisticated (clipped percentage?) in your awk
above, but is it worth 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.
I'm not sure. I just wanted to mention the option of auto-adjusting the value of the parameter. In real life, any number (fixed or adjusted) would be wrong anyway since you don't know on what hardware the code will be run.
tree \ | ||
vim \ | ||
&& dnf clean all \ | ||
&& rm -rf /var/cache/yum | ||
|
||
RUN dnf update -y \ | ||
&& dnf install -y \ | ||
https://apache.jfrog.io/artifactory/arrow/almalinux/$(cut -d: -f5 /etc/system-release-cpe | cut -d. -f1)/apache-arrow-release-latest.rpm \ | ||
https://apache.jfrog.io/artifactory/arrow/almalinux/9/apache-arrow-release-latest.rpm \ |
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 means we would have to keep an eye on this line after upgrading to Alma 10
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.
Docker layer cache works better with fixed expressions (though I suppose if you are changing the base FROM
, that's out the window anyway...)
@@ -234,7 +239,7 @@ RUN dnf install -y 'dnf-command(config-manager)' \ | |||
# FIXME: Keep only binaries, and not devel libraries | |||
RUN dnf update -y \ | |||
&& dnf install -y \ | |||
https://apache.jfrog.io/artifactory/arrow/almalinux/$(cut -d: -f5 /etc/system-release-cpe | cut -d. -f1)/apache-arrow-release-latest.rpm \ | |||
https://apache.jfrog.io/artifactory/arrow/almalinux/9/apache-arrow-release-latest.rpm \ |
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.
Same observation on the fixed version number.
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.
And same reply :-)
@@ -1,4 +1,3 @@ | |||
version: "3.9" |
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 have built and tried testing the new code in my development environment at USDF's machine sdfqserv001
(RedHat 8), and the integration test couldn't be started because of:
gapon@sdfqserv001 qserv (tickets/DM-48903) $ qserv up --dashboard-port=25089 --http-frontend-port 4049
/usr/lib/python3.6/site-packages/requests/__init__.py:91: RequestsDependencyWarning: urllib3 (1.26.20) or chardet (3.0.4) doesn't match a supported version!
RequestsDependencyWarning)
yaml: map merge requires map or sequence of maps as the value
gapon@sdfqserv001 qserv (tickets/DM-48903) $ python3 --version
Python 3.6.8
Could this be a problem of the Python version?
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.
Unrelated I believe -- this version
is docker compose yaml-format version and not at all Python-related. The compose version
directive has been deprecated in compose v2, and was generating a warning.
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'll need to do something about your ability to run unit tests before we merge this, though... It could be that system python 3.6 library suite is now too old for the qserv
Python command, which means you'd need to set up a conda or venv with something newer to run it outside the build container. I wonder why it broke for you on this PR, though?
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 suspect, the problem is with the version of Docker CLI and Docker compose plugin which is much newer in Alma 9 versus what I can use on RedHat 8
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.
For the record, the problem was solved by upgrading the local version of the plugin to:
dnf info docker-compose-plugin
Last metadata expiration check: 2:53:51 ago on Wed 12 Feb 2025 02:47:20 PM PST.
Installed Packages
Name : docker-compose-plugin
Version : 2.27.0
Release : 1.el8
...
This makes the local integration test happy.
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.
LGTM
Python3.12 and other changes, including:
uv
in place ofpip
cmake
3.31.5log4cxx
1.3.1sphgeom
andlog
to their latest respectivemain
sruff
tool to build container (ruff-ed sources upcoming on separate ticket)