search

AIDASoft / podio

PODIO
GNU General Public License v3.0
23 stars 57 forks source link

python: install to a proper libdir prefix #599

Closed veprbl closed 2 months ago

veprbl commented 3 months ago

Currently, the libraries are installed to $PREFIX/python, which is a non-standard prefix. Standard prefix looks more like $PREFIX/lib/python3.XX/site-packages.

BEGINRELEASENOTES

ENDRELEASENOTES

tmadlener commented 3 months ago

Linking https://github.com/key4hep/EDM4hep/issues/220 just to have a consistent way of setting this for both. Maybe @andresailer has a preference of the exact technicalities of how we achieve effectively the same path?

andresailer commented 3 months ago

I think we need to harmonise a few places where we install python things?

  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages/podio_class_generator.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages/podio_schema_evolution.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/reading.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/__init__.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/base_reader.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/frame_iterator.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/pythonizations
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/pythonizations/__init__.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/pythonizations/collection_subscript.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/pythonizations/utils
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/pythonizations/utils/__init__.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/pythonizations/utils/pythonizer.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/base_writer.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/sio_io.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/frame.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/__version__.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio/root_io.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages/podio_version.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio_gen
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio_gen/__init__.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio_gen/cpp_generator.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio_gen/generator_utils.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio_gen/julia_generator.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio_gen/podio_config_reader.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//podio_gen/generator_base.py
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/selection.xml.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/MutableStruct.jl.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/SIOBlock.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Obj.cc.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Object.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Object.cc.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Collection.cc.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Obj.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/CollectionData.cc.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/CollectionData.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/MutableObject.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Component.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Data.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/DatamodelDefinition.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Interface.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/sioblocks.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/interface.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/utils.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/collections.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/julia_helpers.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/implementations.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/iterator.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/macros/declarations.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/CMakeLists.txt
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/ParentModule.jl.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/SIOBlock.cc.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Component.cc.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/MutableObject.cc.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/Collection.h.jinja2
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/schemaevolution
  -- Installing: /home/runner/work/podio/podio/install/lib64/python3.10/site-packages//templates/schemaevolution/EvolvePOD.h.jinja2
tmadlener commented 3 months ago

I think most of this is OK? And since EDM4hep builds downstream with this at least what we install and offer via CMake to downstream is consistent.

What would you place differently?

andresailer commented 3 months ago

I would put everything under site-packages/podio. Probably would mean some site-packages/podio/podio ?

tmadlener commented 3 months ago

Ah yes, that would make sense I think. Or maybe put the templates into podio_gen? That would then only leave

at the top level directory.

hegner commented 3 months ago

sounds like a better convention to me

andresailer commented 3 months ago

Rename podio_gen to gen, and place inside podio/gen Change imports to start with from podio. ?

tmadlener commented 3 months ago

And then still install to <the-rest>/site-packages/podio? I.e. still having site-packages/podio/podio?

Regardless, I would move the templates into podio_gen (or podio/gen).

hegner commented 3 months ago

Yes, podio/gen would be best

tmadlener commented 3 months ago

I think I just remembered why we have podio_gen and not podio/gen, see https://github.com/AIDASoft/podio/pull/511 @jmcarcell bringing podio_gen back into podio/gen would mean quite a bit of slow down for the generation, right?

hegner commented 3 months ago

Damn. Indeed that would be a bit awful...

andresailer commented 3 months ago

Let's leave it as is then.

jmcarcell commented 3 months ago

Keeping podio_gen separated from the rest lets us run without loading anything related generation and generate without loading the rest of podio so I think it's fine as it is.

Indeed we have many repos that use python that we may also want to change:

$ /cvmfs/sw-nightlies.hsf.org/key4hep/releases/2024-05-09/x86_64-almalinux9-gcc11.4.1-opt# find . -maxdepth 4 -iname "python"
./dual-readout/51f9766bb3f9e80644bb032b98f1f5b1d4cb0327_develop-xnlqg4/python
./edm4hep/181745967206fa03b1c6a9aa26b947d4b74aeaf4_develop-mqna4k/python
./gaudi/38.1-kpe2ng/python
./k4clue/efbd91e8accafb95441e6106558db8f9f84639d6_develop-eupqww/python
./k4fwcore/6c55ebd2230133761edf0f45ea69e84c8434a6cd_develop-dbkcrr/python
./k4gen/bf49029a914e7f5fbd09a2954d6dc449978b0c9f_develop-jn3s52/python
./k4marlinwrapper/0493cfd4516a3f495c890160d0e45b69bc0ddeaa_develop-snuqhn/python
./k4projecttemplate/c1af0d0e4eb8d2713cb2caae3c4fe8a245274ca0_develop-3wbpbv/python
./k4reccalorimeter/255127a703c3b623bdde479ddefdf68cf90ae452_develop-q4ggwa/python
./k4rectracker/871276d03db4d1a92f600eb41ea1213220a7cdf2_develop-vrso36/python
./k4simdelphes/7370e7edeb133680772d5f534b8e88ce0c61644c_develop-l3wfci/python
./k4simgeant4/89f0ac0f510b5dd8715537e04782f70bf478ad2f_develop-w46v7x/python
./lcio/e8c5032e261708f03514379decd7375038722025_develop-rq2agf/python
./podio/a18229b9b1f1db6530dec0bd7c6e48f26a0a5f9f_develop-56qqgq/python
./python
./python/3.10.13-vrpxgy/bin/python
./fccanalyses/6d43e284688a0f33f2a5186d69f7f13544e7fb21_develop-ojqkm4/python
./fccsw/e103621518b26bb18f5b928fcc1dc8c5c8c0d737_develop-gmftmc/python

After this change the spack recipe needs to be updated (https://github.com/spack/spack/pull/44291) and I think k4_local_repo also needs to be updated and that should be all if I'm not missing anything.

haampie commented 3 months ago

Got here through the Spack PR.

This

${CMAKE_INSTALL_LIBDIR}/python${Python_VERSION_MAJOR}.${Python_VERSION_MINOR}/site-packages

looks sensible, but unclear if CMAKE_INSTALL_LIBDIR is guaranteed to be correct, since Python may use lib while CMake uses lib64 or the other way around.

The source of truth is 

python3 -c "import sysconfig, sys; sys.stdout.write(sysconfig.get_path('platlib'))"

which CMake provides through Python_SITEARCH if you're using FindPython.cmake.

The issue is that this is an absolute path to the system site packages dir, idk how you get just the relative bits lib/python3.11/site-packages in a canonical way from there.

I would recommend letting users pass the Python install dir as a cmake variable, cause in Spack we expose <package prefix>/<python platdir> so users can pass it to build systems, and I can imagine that non-Spack users may still wanna choose where they put the Python package (maybe the python package goes in a venv but the rest in CMAKE_INSTALL_PREFIX?)

veprbl commented 3 months ago

@haampie See also the original version. It's probably not platlib, but purelib, and would not need a multilib prefix. Python_SITEARCH is probably a bad idea, it would try to install to a system-wide python location.

jmcarcell commented 3 months ago

Than after the comments from @haampie, I think the answer would be something like:

find_package(Python3 REQUIRED)
set(PYTHON_LIB_DIR lib)
if("${Python3_SITEARCH}" MATCHES "/lib64/")
  set(PYTHON_LIB_DIR lib64)
endif()

set(podio_PYTHON_INSTALLDIR "${CMAKE_INSTALL_PREFIX}/${PYTHON_LIB_DIR}/python${Python3_VERSION_MAJOR}.${Python3_VERSION_MINOR}/site-packages")
set(podio_PYTHON_INSTALLDIR ${podio_PYTHON_INSTALLDIR} PARENT_SCOPE)

For the rest of the comment I think our users don't care where the python packages are installed as long as it works.

tmadlener commented 2 months ago

I have now implemented the following:

@veprbl unless you have any objections, I would rebase this onto the latest master/main before we merge it.

veprbl commented 2 months ago

Thanks for taking care of this, @tmadlener. It looks fine to me.

tmadlener commented 2 months ago

I checked that this works as expected locally with spack and extends("python"). I will open a PR shortly for picking this up there.