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

correctly determine INTEL_ROOT_DEFAULT using MKLROOT env variable #1706

Merged
merged 2 commits into from
May 24, 2024

Conversation

alexlnkp
Copy link
Contributor

As mentioned in the issue #1703 , MKLROOT env variable is set to /opt/intel/oneapi/mkl/latest, which makes sense. However, CMakeLists.txt tries to find the root of INTEL by going one dir up. set(INTEL_ROOT_DEFAULT $ENV{MKLROOT}/... This wouldn't work, as the MKLROOT points to the latest, not oneapi. Therefore, to fix this - it's better to go 3 directories up, so that if MKLROOT is /opt/intel/oneapi/mkl/latest, it would correctly point to the root of INTEL.

@minhthuc2502
Copy link
Collaborator

minhthuc2502 commented May 23, 2024

Hello, this change will break the compilation on Windows. For the environment like Arch, you can set variable environment like INTELROOT or MKLROOT to make it work as you did in the issue (this action is expected for certain OS). No PR is needed.

@alexlnkp
Copy link
Contributor Author

Hello, this change will break the compilation on Windows. For the environment like Arch, you can set variable environment like INTELROOT or MKLROOT to make it work as you did in the issue (this action is expected for certain OS). No PR is needed.

I mean, i could add a check for the OS and then check the paths, not sure if this would help anyone but me though.
I made this PR for people like me, who used pacman and yay to install all of the stuff the CTranslate2 needs, so i believe that this idea is more or less valid for at least the arch users.
Please let me know if some of the suggestions i made are valid.

@minhthuc2502
Copy link
Collaborator

minhthuc2502 commented May 23, 2024

If you don't set anything, INTEL_ROOT_DEFAULT will be /opt/intel . Otherwise, in your case, MKLROOT env variable is set to /opt/intel/oneapi/mkl/latest, then you set set(INTEL_ROOT_DEFAULT $ENV{MKLROOT}/../../..), the INTEL_ROOT_DEFAULT would be /opt/intel. Something I missed here? Why you have to set MKLROOT?

@alexlnkp
Copy link
Contributor Author

If you don't set anything, INTEL_ROOT_DEFAULT will be /opt/intel . Otherwise, in your case, MKLROOT env variable is set to /opt/intel/oneapi/mkl/latest, then you set set(INTEL_ROOT_DEFAULT $ENV{MKLROOT}/../../..), the INTEL_ROOT_DEFAULT would be /opt/intel. Something I missed here? Why you have to set MKLROOT?

That's the neat part! I didn't set MKLROOT. The package which is used to install the intel MKL stuff sets the env variable MKLROOT automatically via the INTEL's shell script, that was the main cause of the #1703 .
The shell script in question is /opt/intel/oneapi/mkl/latest/env/vars.sh

@minhthuc2502
Copy link
Collaborator

I see, You can do like this:

elseif(DEFINED ENV{MKLROOT})
  if(WIN32)
    set(INTEL_ROOT_DEFAULT $ENV{MKLROOT}/..)
  else()
   # Other system like arch set env MKLROOT by default
   set(INTEL_ROOT_DEFAULT $ENV{MKLROOT}/../../..) 

@alexlnkp
Copy link
Contributor Author

alexlnkp commented May 23, 2024

I see, You can do like this:

elseif(DEFINED ENV{MKLROOT})
  if(WIN32)
    set(INTEL_ROOT_DEFAULT $ENV{MKLROOT}/..)
  else()
   # Other system like arch set env MKLROOT by default
   set(INTEL_ROOT_DEFAULT $ENV{MKLROOT}/../../..) 

Alrighty, i'll do that.
Though, do you want me to squash this change into previous commit so it's just a single-commit PR or should i just have 2 commits here?

@minhthuc2502
Copy link
Collaborator

minhthuc2502 commented May 23, 2024

Don't worry about squashing. I'll do when merge it.

@minhthuc2502 minhthuc2502 merged commit 3b248f1 into OpenNMT:master May 24, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants