Skip to content
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

Move to manylinux_2_28 base image for wheels #3306

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

JCGoran
Copy link
Collaborator

@JCGoran JCGoran commented Jan 10, 2025

  • use new Docker image tags (manylinux_2_28_x86_64 and manylinux_2_28_aarch64) in CI for building wheels
  • use image-provided flex, mpich, and openmpi instead of building from source (still need custom readline and ncurses as the *-static versions do not use -fPIC)
  • unify MPI headers when building wheels
  • remove BBP-specific tests for wheels
  • show warning in Python wrapper if host machine is using GCC<10

@JCGoran JCGoran changed the title [WIP] Move to manylinux_2_28 base image [WIP] Move to manylinux_2_28 base image for wheels Jan 10, 2025
@pramodk
Copy link
Member

pramodk commented Jan 10, 2025

I didn’t check the details but just asking for clarity - moving to 2.28 changed anything about old platforms supported?

@JCGoran
Copy link
Collaborator Author

JCGoran commented Jan 10, 2025

I didn’t check the details but just asking for clarity - moving to 2.28 changed anything about old platforms supported?

This is mostly in reference to #1963 (comment) to assess how much work is needed.
The change would only have an impact on users who have older systems, but would like to run a newer version of NEURON (9 and above), and only via wheels (source/Spack builds would remain unaffected), i.e. personal machines, since cluster deployments should anyway use a Spack/source install (for performance reasons).

The new min. system requirements would be:

  • Debian 10+ (10 reached EOL in July 2024)
  • Ubuntu 18.10+ (the previous LTS version, 18.04.6, reached EOL in May 2023)
  • Fedora 29+ (reached EOL years ago)
  • CentOS/RHEL 8+ (CentOS 7 reached EOL July 2024)

Copy link

✔️ 952d3d9 -> Azure artifacts URL

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.07%. Comparing base (74dcdd5) to head (4ffe46c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3306   +/-   ##
=======================================
  Coverage   67.07%   67.07%           
=======================================
  Files         571      571           
  Lines      111055   111055           
=======================================
  Hits        74488    74488           
  Misses      36567    36567           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pramodk
Copy link
Member

pramodk commented Jan 10, 2025

Thanks for checking. Those all looks good! 👍

@JCGoran
Copy link
Collaborator Author

JCGoran commented Jan 13, 2025

Okay, some additional findings about this:

  • GCC major versions are not backwards compatible, i.e. we cannot reliably compile and link mod files w/GCC vOlder if we compiled a manylinux wheel w/GCC vNewer (if we didn't have to compile and link anything on the host machine, but just use it, we'd most likely be fine)
  • the version of the compiler in the manylinux image is actually too new for us (GCC 14); fortunately, we can install GCC 10 from the repos and use that as the compiler toolchain
  • why GCC 10? Because it's available for installation (may not be default) on at least Ubuntu 20.04 LTS, Debian 11, and (trivially) Almalinux 8, which are all rather old, but best be safe than sorry
  • I've also changed the min. version of GCC checked by the _binwrapper.py script (from 9 to 10) because we used to build things with GCC 10 anyway, and due to the first point above, there is no guarantee that NEURON would compile and link mod files properly w/ GCC 9
  • with the above in mind, we should also be more explicit about the min. version of GCC needed on the user system in the docs

In summary, I think the move to manylinux_2_28 without too many changes is more than feasible.

Copy link

✔️ 44ef470 -> Azure artifacts URL

@JCGoran JCGoran changed the title [WIP] Move to manylinux_2_28 base image for wheels Move to manylinux_2_28 base image for wheels Jan 13, 2025
@JCGoran JCGoran marked this pull request as ready for review January 13, 2025 18:53
Copy link

✔️ 4ffe46c -> Azure artifacts URL

@@ -1 +0,0 @@
../../docs/install/python_wheels.md
Copy link
Collaborator Author

@JCGoran JCGoran Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly to fix an issue with Podman on MacOS:

Error: invalid symlink "/var/tmp/libpod_builder742187951/build/README.md" -> "../../docs/install/python_wheels.md"

It also makes it more GitHub-friendly since GitHub doesn't follow symlinks, it just displays where it's pointing to; now it's actually clickable.

Copy link

✔️ fae53da -> Azure artifacts URL

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many details I'm not familiar with. But if it works...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants