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

Fixed CMake was not looking for NumPy in virtual envs #133

Merged
merged 2 commits into from
Feb 25, 2021
Merged

Conversation

gasse
Copy link
Member

@gasse gasse commented Feb 23, 2021

Pull request checklist

Two issues:
1 - the CMake errors are garbage when Python/Numpy are not found by FindNumpy.cmake
2 - FindNumpy.cmake seems to be incompatible with virtual environments, at least the ones on compute-canada. It finds the absolute path to the python interpreter, but then it fails to import numpy. I have the same behaviour from the command line:
/myvirtualenvpath/bin/python -c 'import numpy' succeeds
/systempath/bin/python3.8 -c 'import numpy' fails
Although /myvirtualenvpath/bin/python is a symlink to /systempath/bin/python3.8.

Proposed implementation

1 - call the FindNumPy.cmake script with a call to find_package(NumPy REQUIRED) instead of include("cmake/FindNumPy.cmake"). I believe this is the intended use, and this way the script stops early on if, say, it does not find the Python interpreter. Otherwise it was always going on, which triggered the weird messages in #132
2 - instead of obtaining python by calling find_package(PythonInterp REQUIRED), get the default python with execute_process(COMMAND "which" "python" ...). This solves my problem on compute-canada, this way numpy is properly found within the virtual env. Not sure why it does so, nor why we even need to find an absolute path to the python interpreter for obtaining the numpy include dirs.

Note that I have tried CMake's FindPython, but it had the same issue: incompatible with my virtual env. Even if the doc says it should work with virtual envs.

So I think we should merge 1) for sure, and 2) only if it does not break CI and ideally every one else's builds.

CMakeLists.txt Outdated
@@ -14,6 +14,9 @@ project(
DESCRIPTION "Extensible Combinatorial Optimization Learning Environments"
)

# path to e.g. findNumPy module
set(CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake/Modules)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to add this. It overrides CMake search path, which may lead to problems outside of your use case.

Comment on lines 42 to 45
if(NumPy_FIND_REQUIRED)
find_package(PythonInterp REQUIRED)
else()
find_package(PythonInterp)
endif()
execute_process(COMMAND "which" "python"
RESULT_VARIABLE PYTHONINTERP_FOUND
OUTPUT_VARIABLE PYTHON_EXECUTABLE
OUTPUT_STRIP_TRAILING_WHITESPACE)
Copy link
Member

Choose a reason for hiding this comment

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

There is more to find_package that finding the Python interpreter. However, we can tell find_package where too look by setting PYTHON_EXECUTABLE.
This is what we were doing here. Perhaps we simply need to change that for a which python instead of find_program?

@AntoinePrv
Copy link
Member

I have the same behaviour from the command line:
/myvirtualenvpath/bin/python -c 'import numpy' succeeds
/systempath/bin/python3.8 -c 'import numpy' fails
Although /myvirtualenvpath/bin/python is a symlink to /systempath/bin/python3.8.

This looks normal. Every time you do a virtual environment, you don't want to copy Python, but you also don't want to see global packages. Hence I believe Python know if it is symlinked to know where to look for packages.
So perhaps this is simply an issue of CMake resolving the symlink.

Note that I have tried CMake's FindPython, but it had the same issue: incompatible with my virtual env. Even if the doc says it should work with virtual envs.

So maybe it our find_progam that mess up the symlink.

@gasse
Copy link
Member Author

gasse commented Feb 24, 2021

Less is more :P

This works on compute-canada, could you give it a try on your side ?

Copy link
Member

@AntoinePrv AntoinePrv left a comment

Choose a reason for hiding this comment

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

This is much simpler

@AntoinePrv AntoinePrv merged commit 16ab333 into master Feb 25, 2021
@AntoinePrv AntoinePrv deleted the cmake-fix branch February 25, 2021 14:24
@AntoinePrv
Copy link
Member

This turn out to be working poorly for me on MacOS and Linux (depspite working in CI).
@gasse What value do you get for cat build/CMakeCache | grep Python_EXECUTABLE ?

I'm thinking of adding:

execute_process(
    COMMAND "python" "-c" "import sys; print(sys.executable)"
    OUTPUT_VARIABLE Python_EXECUTABLE
)

In order to tell CMake "Just take the one in my path. Period. No fancy looking. No resolving symlink"

Can you confirm that python -c 'import sys; print(sys.executable)' would give the correct interpreter for you?

@gasse
Copy link
Member Author

gasse commented Feb 25, 2021

Yes I don't know why we need this fancy search for the python executable anyway...
python -c 'import sys; print(sys.executable)' indeed returns the correct path in my virtual environment.

(build-env) [gassmaxi@beluga5 ecole]$ python -c 'import sys; print(sys.executable)'
/home/gassmaxi/ecole/build-env/bin/python

As for my CMakeCache:

(build-env) [gassmaxi@beluga5 ecole]$ cat build/CMakeCache.txt | grep Python_EXECUTABLE
_Python_EXECUTABLE:INTERNAL=/home/gassmaxi/ecole/build-env/bin/python3.8

@AntoinePrv
Copy link
Member

Yes I don't know why we need this fancy search for the python executable anyway...

I think it's more like if you install an application, it should more link with system Python.

python -c 'import sys; print(sys.executable)' indeed returns the correct path in my virtual environment

So if you go

cmake -B build -D Python_EXECUTABLE="$(python -c 'import sys; print(sys.executable)')"

It still works for you?

@gasse
Copy link
Member Author

gasse commented Feb 26, 2021

Yes it works

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