conda-forge / intel_repack-feedstock

A conda-smithy repository for intel_repack.
BSD 3-Clause "New" or "Revised" License
6 stars 19 forks source link

`intel-openmp` should conflict orther impls #59

Open ax3l opened 1 year ago

ax3l commented 1 year ago

I noticed something similar to the MPI issue with OpenMP since recently: Is it possible that, similar to the recent intel MPI issue, there is another issue that some Python packages started to pull in intel-openmp (or mkl, which requires intel-openmp) and do not clearly mark it as alternative to LLVM OpenMP and GNU OpenMP?

I think that intel-openmp needs to know about libgomp, llvm-openmp and the libomp140.x86_64.dll that is shipped in Visual Studio/VC4 (?) of Conda-Forge? and conflict properly. https://conda-forge.org/docs/maintainer/knowledge_base.html#openmp

ax3l commented 1 year ago

I see this currently in Windows builds, where I build with clang-cl for modern OpenMP support using llvm-openmp. This results at runtime in issue of the form

OMP: Error #15: Initializing libomp.dll, but found libomp.dll already initialized.
OMP: Hint This means that multiple copies of the OpenMP runtime have been linked into the program. That is dangerous, since it can degrade performance or cause incorrect results. The best thing to do is to ensure that only a single OpenMP runtime is linked into the process, e.g. by avoiding static linking of the OpenMP runtime in any library. As an unsafe, unsupported, undocumented workaround you can set the environment variable KMP_DUPLICATE_LIB_OK=TRUE to allow the program to continue to execute, but that may cause crashes or silently produce incorrect results. For more information, please see http://openmp.llvm.org/
Fatal Python error: Aborted

since recently. I think the reason is that some Python package I depend on pulls in intel-openmp although it is incompatible with llvm-openmp.

Example: https://github.com/conda-forge/impactx-feedstock/pull/22

ax3l commented 1 year ago

Are some packages in Conda-Forge unconditionally using mkl these days, which depends on Windows on intel-openmp? https://github.com/conda-forge/intel_repack-feedstock/blob/1dd0cb72c6234faef285b3675c5a39bdba54bffe/recipe/meta.yaml#L108-L122

Likely scipy:

MKL seems to be the default BLAS/LAPACK provider on Windows now, which makes as its direct dependency intel-openmp the default OpenMP provider on Windows ... OpenMP not documented as such: https://conda-forge.org/docs/maintainer/knowledge_base.html#openmp

ax3l commented 1 year ago

attn @leofang @oleksandr-pavlyk @isuruf @jeffhammond for help.

I think two things need to be done (but I do not know how):

  1. intel-openmp should conflict with llvm-openmp and libgomp
  2. the inconsistency in the KB should be fixed:
    • Windows OpenMP defaults are undocumented, but unofficially recommended to use llvm-openmp (via clang-cl)
    • Windows BLAS defaults are MKL which requires intel-openmp
leofang commented 1 year ago

Like mpi being a mutex for the MPI implementation, there's an _openmp_mutex for the OpenMP implementation, see how it's used by libgomp: https://github.com/conda-forge/ctng-compilers-feedstock/blob/086beed486c046f9b205bd94ff35eea74d68ee6a/recipe/meta.yaml#L435-L445.

leofang commented 1 year ago

Same for llvm-openmp, the mutex needs to be depended on: https://github.com/conda-forge/openmp-feedstock/blob/13d71e73090821d0323deeb57e99d9a13e6cee47/recipe/meta.yaml#L25-L34

cc: @conda-forge/core for vis

beckermr commented 1 year ago

We need to be careful with the mutexes here. They are not like the mpi ones since we allow swapping openmp at runtime due to ABI compat.

napetrov commented 1 year ago

Are there other packages there we allow swap due to ABI compat? Is there process that we can agree following?

beckermr commented 1 year ago

BLAS is done that way. We need input from @isuruf on this since he did most of the design here.

ax3l commented 1 year ago

Additionally, there seems to be a libomp140.x86_64.dll that is a version of OpenMP shipped in Visual Studio itself (?). I cannot use that be cause the supported OpenMP standard is too old.

As a temporary work-around, I tried to rewrite all my feedstocks to use intel-openmp on Windows. This seems to mostly work fine until I reach: https://github.com/conda-forge/impactx-feedstock/pull/22 Which in some Python pytest tests complains about:

OMP: Error #15: Initializing libiomp5md.dll, but found libomp140.x86_64.dll already initialized.
OMP: Hint This means that multiple copies of the OpenMP runtime have been linked into the program. That is dangerous, since it can degrade performance or cause incorrect results. The best thing to do is to ensure that only a single OpenMP runtime is linked into the process, e.g. by avoiding static linking of the OpenMP runtime in any library. As an unsafe, unsupported, undocumented workaround you can set the environment variable KMP_DUPLICATE_LIB_OK=TRUE to allow the program to continue to execute, but that may cause crashes or silently produce incorrect results. For more information, please see [http://www.intel.com/software/products/support/.](http://www.intel.com/software/products/support/)
Fatal Python error: Aborted

For building, I find OpenMP with CMake's FindOpenMP.cmake.

leofang commented 1 year ago

We need to be careful with the mutexes here. They are not like the mpi ones since we allow swapping openmp at runtime due to ABI compat.

@beckermr I am not sure I follow. Regardless of MPI/OpenMP/BLAS, the purpose of the mutexes is the same: Disallowing multiple implementations from being installed to the same conda environment. How the mutex is implemented is an implementation detail that only package maintainers need to know.

In the case of MPI, we need to add a string identifier to mpi for a new MPI implementation.

In the case of BLAS, we need to register the BLAS implementation in the blas-feedstock.

In the case of OpenMP, for some reason unclear to me the mutex publishing happens in two feedstocks, ctng-compiler (for gomp) and _openmp_mutex (for llvm-openmp).

But in the end, all such packages work with the same concept. Am I missing something?

It's worth noting that @h-vetinari also reported this very same issue 2 years ago: https://github.com/conda-forge/_openmp_mutex-feedstock/issues/7 We definitely should fix it.

isuruf commented 1 year ago

Regarding ABI, we have intel-openmp because llvm-openmp does not have the VCOMP symbols in them which leads to pytorch on windows breaking. VCOMP is old and going away in favour of LLVM openmp (even MSVC supports LLVM openmp now). We really should have a pytorch win build in conda-forge and then we can remove intel-openmp. Intel and LLVM OpenMP share code except for the VCOMP compatibility options.

Yes, we can have intel-openmp and llvm-openmp conflict with each other for now.

milubin commented 1 year ago

@isuruf As mentioned above MKL depends on intel-openmp on Windows. Does intel-openmp need to be replaced with llvm-openmp in MKL too?

ax3l commented 1 year ago

Hi, is there any progress on this, e.g., marking the conflict for `intel-openmp? :)

Krande commented 5 months ago

Hi, we are also seeing issues for us when developing Windows support for Code Aster due to the clobbering of files when mixing intel-openmp and llvm-openmp when trying to use intel-fortran-rt (depends on llvm-openmp) together with mkl (depends on intel-openmp). This is ultimately causing dll issues for us. I can fix it manually by doing mamba remove intel-openmp llvm-openmp --force and then do mamba install intel-openmp. But that's not really solving our issue :(

Would it be possible to switch from intel to llvm openmp for mkl or llvm to intel for intel-fortran-rt? Alternatively, create variants of mkl and intel-fortran-rt that supports both?

I can try to help with a PR with a little guidance.

ZzEeKkAa commented 2 weeks ago

I've added constraint to conflict with llvm-intel please let me know if it partially solves the problem