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

Revert installing Python module systemwide. #473

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

1uc
Copy link
Contributor

@1uc 1uc commented Sep 11, 2023

The lines

install(
  TARGETS _morphio
  LIBRARY DESTINATION ${Python3_SITEARCH}/morphio/
)

cause CMake to find the directory where Python modules are installed. This would be the systemwide directory if one is simply building with the systemwide Python. If one has a venv active it'll install into that virtual environment.

Note that CMAKE_INSTALL_PREFIX doesn't affect where the module will be installed. Therefore, this will cause build/installation failure.

Installing into a venv is questionable, because that's the responsibility of the Python package manager. Which will do so correctly without the additional CMake install statement.

Finally, even if the install succeeds, it only installs the lib*.so without any __init__.py files. Therefore, it leads to a broken installation, e.g.

python -c "import morphio; morphio.Soma"

will successfully import morphio but there won't be any morphio.Soma, because that's handled via __init__.py

The lines

    install(
      TARGETS _morphio
      LIBRARY DESTINATION ${Python3_SITEARCH}/morphio/
    )

cause CMake to find the directory where Python modules are installed.
This would be the systemwide directory if one is simply building with
the systemwide Python. If one has a venv active it'll install into that
virtual environment.

Note that `CMAKE_INSTALL_PREFIX` doesn't affect where the module will be
installed. Therefore, this will cause build/installation failure.

Installing into a venv is questionable, because that's the
responsibility of the Python package manager. Which will do so
correctly without the additional CMake  `install` statement.

Finally, even if the install succeeds, it  only installs the `lib*.so`
without any `__init__.py` files. Therefore, it leads to a broken
installation, e.g.

    python -c "import morphio; morphio.Soma"

will successfully import `morphio` but there won't be any
`morphio.Soma`, because that's handled via `__init__.py`
@1uc
Copy link
Contributor Author

1uc commented Sep 12, 2023

Here's the steps to reproduce on BB5:

module load unstable gcc cmake python hdf5
cmake -DCMAKE_INSTALL_PREFIX=build/install -DMorphIO_WERROR=Off -B build
cmake --build build --parallel --target install

which fails with:

CMake Error at binds/python/cmake_install.cmake:60 (file):
  file cannot create directory:
  /gpfs/bbp.cscs.ch/ssd/apps/bsd/2023-02-23/stage_externals/install_gcc-12.2.0-skylake/python-3.10.8-rvn6l5/lib/python3.10/site-packages/morphio.
  Maybe need administrative privileges.
Call Stack (most recent call first):
  cmake_install.cmake:80 (include)

@mgeplf mgeplf merged commit bdccf36 into master Sep 12, 2023
20 checks passed
@mgeplf mgeplf deleted the 1uc/revert-python-install-commands branch September 12, 2023 06:34
@mgeplf
Copy link
Contributor

mgeplf commented Sep 12, 2023

thanks for finding this, and the fix.

@1uc
Copy link
Contributor Author

1uc commented Sep 12, 2023

(For posterity.)

Here's a variation that seems to show that even if there isn't a permission issue the lines:

install(
  TARGETS _morphio
  LIBRARY DESTINATION ${Python3_SITEARCH}/morphio/
)

are likely not what we expect. Here's the required step on BB5:

module load unstable gcc cmake python hdf5
# Ensure that we have write permissions during installation of `_morphio*.so`.
python -m venv venv
source venv/bin/activate
cmake -DCMAKE_INSTALL_PREFIX=build/install -DMorphIO_WERROR=Off -B build

We can now run and observe:

$ cmake --build build --parallel --target install
...
-- Installing: /gpfs/bbp.cscs.ch/home/groshein/MorphIO/venv/lib/python3.10/site-packages/morphio/_morphio.cpython-310-x86_64-linux-gnu.so

$ ls /gpfs/bbp.cscs.ch/home/groshein/MorphIO/venv/lib/python3.10/site-packages/morphio/
_morphio.cpython-310-x86_64-linux-gnu.so

$ which python 
~/MorphIO/venv/bin/python

$ cd # to avoid the folder `morphio`.
$ python -c "import morphio"
$ python -c "import morphio; morphio.Soma"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: module 'morphio' has no attribute 'Soma'
$ pip uninstall morphio
WARNING: Skipping morphio as it is not installed.

To summarize, even if we have permissions, those lines would install a version of morphio which can be imported, but not used. Moreover, it can't be uninstalled via pip essentially breaking our virtual environment.

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