Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

Improve Python wheel support #785

Merged
merged 10 commits into from
Mar 7, 2022
Merged

Improve Python wheel support #785

merged 10 commits into from
Mar 7, 2022

Conversation

olupton
Copy link
Contributor

@olupton olupton commented Feb 28, 2022

Description
This PR tweaks a few things to improve CoreNEURON support in NEURON's Python wheels.

  • Use new NRN_WHEEL_BUILD option to avoid adding a build flag that is not widely supported. This option is added in GPU wheel improvements neuronsimulator/nrn#1661.
  • Make nrnivmodl -coreneuron fail in GPU builds if a non-NVC++ compiler is used.
  • Make nrnivmodl -coreneuron fail in GPU builds if a different NVC++ version is used to the one used to build the wheel.
  • Move some pre-existing logic into a new cnrn_parse_version CMake function; use it to get major.minor version numbers from CUDA and NVHPC versions that have .patch parts too.
  • Modify CORENEURON_LIB_LINK_FLAGS to use -L$(libdir), which catches an edge case when a patched libnrniv.so is being used so the relevant path was not already set.
  • Make the logic for translating CMake target dependencies into linker flags more verbose.
  • Don't print single-MPI paths in dynamic MPI mode.
  • Don't add -I for MPI headers in dynamic MPI builds.

Progress towards #672.

Use certain branches for the SimulationStack CI
CI_BRANCHES:NEURON_BRANCH=olupton/wheel-testing

@olupton olupton marked this pull request as draft February 28, 2022 10:40
@olupton olupton force-pushed the olupton/wheel-tweaks branch from c56614e to e86ed6c Compare February 28, 2022 11:20
@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2022

Codecov Report

Merging #785 (5e96bf4) into master (856cea4) will not change coverage.
The diff coverage is n/a.

❗ Current head 5e96bf4 differs from pull request most recent head 5079855. Consider uploading reports for the commit 5079855 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #785   +/-   ##
=======================================
  Coverage   56.00%   56.00%           
=======================================
  Files         108      108           
  Lines        9003     9003           
=======================================
  Hits         5042     5042           
  Misses       3961     3961           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 856cea4...5079855. Read the comment docs.

@olupton olupton closed this Mar 2, 2022
@olupton olupton reopened this Mar 2, 2022
@olupton olupton force-pushed the olupton/wheel-tweaks branch from 5079855 to 28710a0 Compare March 2, 2022 09:36
@olupton olupton force-pushed the olupton/wheel-tweaks branch from e2cd7fe to 960360e Compare March 3, 2022 15:44
@olupton olupton marked this pull request as ready for review March 3, 2022 16:12
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Mar 3, 2022
@olupton olupton requested review from pramodk and alexsavulescu March 4, 2022 09:30
Copy link
Collaborator

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

LGTM!

CMakeLists.txt Show resolved Hide resolved
@pramodk pramodk merged commit f3d4615 into master Mar 7, 2022
@pramodk pramodk deleted the olupton/wheel-tweaks branch March 7, 2022 13:35
olupton added a commit to neuronsimulator/nrn that referenced this pull request Mar 7, 2022
- Test script: use intel-oneapi-mpi when testing wheels on BB5.
- Add `NRN_WHEEL_BUILD` variable for use inside the CMake configuration when
  tuning build options for Python wheels.
- Bump submodule past BlueBrain/CoreNeuron#785.
- Don't print single-MPI variables when dynamic MPI is enabled.
- When running `nrnivmodl` on user machines in GPU wheel installations, modify
  libnrniv.so before linking to it:
  - Remove link dependencies on bundled NVIDIA runtime libraries, so these are
    not duplicated when the same runtimes from the user's system are implicitly
    linked by `nvc++`.
  - Require `patchelf` on the user's system to achieve this.
- Don't add single-MPI include directories in dynamic MPI builds.
- Make the CMake logic to translate target dependencies into compiler flags
  more verbose.
- Build GPU wheels with `-tp=haswell` instead of the default (`-tp=host`), but
  remove this from the Makefiles used by `nrnivmodl` on user machines.
alexsavulescu added a commit to neuronsimulator/nrn that referenced this pull request Mar 8, 2022
- Test script: use intel-oneapi-mpi when testing wheels on BB5.
- Add `NRN_WHEEL_BUILD` variable for use inside the CMake configuration when
  tuning build options for Python wheels.
- Bump submodule past BlueBrain/CoreNeuron#785.
- Don't print single-MPI variables when dynamic MPI is enabled.
- When running `nrnivmodl` on user machines in GPU wheel installations, modify
  libnrniv.so before linking to it:
  - Remove link dependencies on bundled NVIDIA runtime libraries, so these are
    not duplicated when the same runtimes from the user's system are implicitly
    linked by `nvc++`.
  - Require `patchelf` on the user's system to achieve this.
- Don't add single-MPI include directories in dynamic MPI builds.
- Make the CMake logic to translate target dependencies into compiler flags
  more verbose.
- Build GPU wheels with `-tp=haswell` instead of the default (`-tp=host`), but
  remove this from the Makefiles used by `nrnivmodl` on user machines.

Co-authored-by: Pramod Kumbhar <[email protected]>
Co-authored-by: Alexandru Săvulescu <[email protected]>
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022
- Use new `NRN_WHEEL_BUILD` option to avoid adding a build flag that is
   not widely supported. This option is added in #1661.
- Make `nrnivmodl -coreneuron` fail in GPU builds if a non-NVC++ compiler is used.
- Make `nrnivmodl -coreneuron` fail in GPU builds if a different NVC++ version is used
   to the one used to build the wheel.
- Move some pre-existing logic into a new `cnrn_parse_version` CMake function;
   use it to get `major.minor` version numbers from CUDA and NVHPC versions that
   have `.patch` parts too.
- Modify `CORENEURON_LIB_LINK_FLAGS` to use `-L$(libdir)`, which catches an
   edge case when a patched `libnrniv.so` is being used so the relevant path was not
   already set.
- Make the logic for translating CMake target dependencies into linker flags more verbose.
- Don't print single-MPI paths in dynamic MPI mode.
- Don't add `-I` for MPI headers in dynamic MPI builds.

CoreNEURON Repo SHA: BlueBrain/CoreNeuron@f3d4615
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants