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

Set CMake hints for path to Python installation if it's direct or indirect dependency (when using CMake >= 3.12) #3282

Conversation

Flamefire
Copy link
Contributor

(created using eb --new-pr)

@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS GROMACS-2020-foss-2019b.eb
  • SUCCESS GROMACS-2021.3-foss-2021a.eb
  • SUCCESS GROMACS-2021.5-foss-2021b.eb
  • SUCCESS GROMACS-2021-foss-2020b.eb
  • SUCCESS GROMACS-2023.1-foss-2022a.eb
  • SUCCESS GROMACS-2023.3-foss-2022a.eb
  • SUCCESS GROMACS-2023.3-foss-2023a.eb
  • SUCCESS GROMACS-2024.1-foss-2023b.eb

Build succeeded for 8 out of 8 (8 easyconfigs in total)
n1459 - Linux RHEL 8.7 (Ootpa), x86_64, Intel(R) Xeon(R) Platinum 8470 (icelake), Python 3.8.13
See https://gist.github.com/Flamefire/d6a8736ee2f5f5cb16b22896fbb80ac2 for a full test report.

@Flamefire
Copy link
Contributor Author

@Micket Anything required here?

@Flamefire
Copy link
Contributor Author

Flamefire commented Jun 6, 2024

@boegel ping

@beeeback
Copy link

beeeback commented Jun 25, 2024

Is this included in new Easybuild v4.9.2?

@Flamefire
Copy link
Contributor Author

Is this included in new Easybuild v4.9.2?

No, this is not even included in the develop branch yet. Otherwise the status on top of this page would say "Merged" instead of "Open"

@boegel boegel modified the milestones: 5.0, release after 4.9.2 Jul 3, 2024
@boegel
Copy link
Member

boegel commented Jul 3, 2024

Looks fine, but this should be tested a bit more broadly than just with GROMACS imho.

Now's a good time to merge this, assuming that the next release will be EasyBuild 5.0, since I wouldn't be surprised if there's some fallout because of this (see #3088, which also shouldn't cause trouble, but I believe it did somewhere)

@boegel boegel changed the title Set CMake hints for Python if added as a dependency Set CMake hints for path to Python instalation if it's included as a dependency (when using CMake >= 3.12) Jul 3, 2024
@Flamefire Flamefire changed the title Set CMake hints for path to Python instalation if it's included as a dependency (when using CMake >= 3.12) Set CMake hints for path to Python installation if it's included as a dependency (when using CMake >= 3.12) Jul 3, 2024
@Flamefire
Copy link
Contributor Author

Looks fine, but this should be tested a bit more broadly than just with GROMACS imho.

GROMACS was used because that is where it fails without this.

I wouldn't be surprised if there's some fallout because of this

This one is much less likely to cause fallout as we basically tell CMake: "Use this!"
If it did use anything else before it was (very likely) just wrong and if it did not, this doesn't change anything.

@boegel
Copy link
Member

boegel commented Jul 3, 2024

@Flamefire Do we fully understand why what we have now fails with GROMACS? If so, can we document this somewhere (either in a dedicated issue, or in here)?

@Flamefire
Copy link
Contributor Author

Flamefire commented Jul 4, 2024

@Flamefire Do we fully understand why what we have now fails with GROMACS? If so, can we document this somewhere (either in a dedicated issue, or in here)?

This is an accompanying PR to #3283 after a report and discussion in Slack

Basically GROMACS changed the default Python search (by CMake) to prefer a virtual env supplied Python. So CMake picked up the Python used for EasyBuild when that was installed in a virtual env, instead of the Python used as a dependency.
For GROMACS specifically we can workaround by passing -DPython3_FIND_VIRTUALENV=STANDARD (which might be a useful default for CMakeMake too), see #3283

This is an alternative that also works by passing the Path to the Python root we want to use to CMake. This way it will consider this first before going through the other defaults (like the active virtual env)

Both PRs fix the issue for GROMACS.

This one should be more reliable especially for CMake options like "prefer virtual env" or "prefer the highest version you find" which we dealt with in the past. So less surprises with this.

@Flamefire Flamefire changed the title Set CMake hints for path to Python installation if it's included as a dependency (when using CMake >= 3.12) Set CMake hints for path to Python installation if it's direct or indirect dependency (when using CMake >= 3.12) Jul 31, 2024
@Flamefire
Copy link
Contributor Author

There just was an issue reported on Slack where OPENFOAM found a system Python:

CMake Error at /cluster/software/CMake/3.23.1-GCCcore-11.3.0/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Python3 (missing: Python3_INCLUDE_DIRS Development.Module)
  (found suitable version "3.11.9", minimum required is "3.10")
Call Stack (most recent call first):
  /cluster/software/CMake/3.23.1-GCCcore-11.3.0/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
  /cluster/software/CMake/3.23.1-GCCcore-11.3.0/share/cmake-3.23/Modules/FindPython/Support.cmake:3181 (find_package_handle_standard_args)
  /cluster/software/CMake/3.23.1-GCCcore-11.3.0/share/cmake-3.23/Modules/FindPython3.cmake:490 (include)
  /cluster/software/ParaView/5.10.1-foss-2022a-mpi/lib64/cmake/paraview-5.10/vtk/VTK-vtk-module-find-packages.cmake:538 (find_package)
  /cluster/software/ParaView/5.10.1-foss-2022a-mpi/lib64/cmake/paraview-5.10/vtk/vtk-config.cmake:150 (include)
  /cluster/software/ParaView/5.10.1-foss-2022a-mpi/lib64/cmake/paraview-5.10/paraview-config.cmake:169 (find_package)
  CMakeLists.txt:7 (FIND_PACKAGE)

So OPENFOAM depends on Paraview which depends on Python but during configure of OPENFOAM ParaView is searched for which searches for Python and finds the system version (even though I'd expect our -DCMAKE_POLICY_DEFAULT_CMP0094=NEW) to avoid that.
As Python is not a direct dependency of OPENFOAM the previous implementation of this wouldn't have added the hints. I hence changed it to always add them when Python is a dependency.

Sometimes Python is a dependency of a dependency and CMake still needs
to find that. So check if Python is loaded and set the hints if it is.
@Flamefire
Copy link
Contributor Author

Closing in favor of #3463

@Flamefire Flamefire closed this Oct 12, 2024
@Flamefire Flamefire deleted the 20240405162414_new_pr_cmakemake branch October 12, 2024 09:40
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.

4 participants