Partial installation with poetry/pip

[marius311]

A number of small generic cmake fixes, and a few things specifically to make https://github.com/marius311/Spt3G.jl possible. (crossed out below were split off and added separately)

1. Makes it possible for the user to override CMAKE_FIND_PACKAGE_PREFER_CONFIG to instead favor MODULE-mode find_package. No change to the current default (CONFIG-mode) if nothing is specified.

   The problem with CONFIG-mode is it seems to sometimes do too good of a job finding *config.cmake files including ones you dont want to use, with no way (that I found) to turn that off. Specifically it was finding $HOME/lib/cmake/Boost.config.cmake via option 4 here (via some built in HINTS or something that have no option to be turned off)

2. It turns out Boost can be built without bzip2 support. Cmake can discover this. This makes spt3g_software still build-able in that case.

3. A NETCDF include file seemed to not be properly specified before (it probably worked fine before if the file was on a system-wide include folder)

4. Add pyproject.toml to record Python dependencies and make the repo PEP 518 compliant. Means you can add a built spt3g_software to some virtual environment with e.g. poetry add -e /path/to/spt3g_software/build and all Python dependencies get installed (haven't played with this outside of poetry but I think in theory pip and some others can do the same). Would be great if this file could be used to keep dependency compatibility bounds up-to-date, as this is not really recorded anywhere afaict.

Happy to take any comments on this. I tried to touch as little as possible while making the Spt3G.jl thing work.

[marius311]

Friendly ping if this could be looked at? Only outstanding comment is the version number wont be right if the repo is checked out as SVN, which I'm not sure needs to be supported?

[arahlin]

Looks good!

[arahlin]

@nwhitehorn care to comment on this? I think this is fine to merge otherwise.

[cnweaver]

1. The CMAKE_FIND_PACKAGE_PREFER_CONFIG change seems useful, as I have also encountered situations in which manually setting this variable is necessary. This seems fine as it is for this PR, but in future we might want to consider designing some way to let users configure this per-(major-)dependency, as they may have different natural defaults or need conflicting settings. Maybe FIND_${PACKAGE}_PREFER_CONFIG for each case where we want to control this? (My use case is for directly installed recent versions of libFLAC which will provide their own CMake config scripts. Unfortunately, these provide a completely different interface than our own FindFLAC, and so cause dismal failure. As a result, defaulting CMAKE_FIND_PACKAGE_PREFER_CONFIG to false for purposes of FLAC is highly desirable.)

2. Generally useful, but see specific comment about further CMake footwork needed.

3. Seems like a natural fix.

4. This does not seem to work yet, and I have some more general concerns about it.

First, running cmake, building the compiled portions as usual, and then trying to use this python install method fails for me:

$ echo $PWD
/home/cweaver/spt3g_software/build
$ poetry add -e $(pwd)

Updating dependencies
Resolving dependencies... (0.0s)

Package 'spt3g' is listed as a dependency of itself.

$ pip install .
Processing /home/cweaver/spt3g_software/build
. . . 
Building wheels for collected packages: spt3g
  Building wheel for spt3g (PEP 517) ... error
  ERROR: Command errored out with exit status 1:
   command: /home/cweaver/ocs/bin/python3 /tmp/tmplww0m12f build_wheel /tmp/tmpfzq8gi9u
       cwd: /tmp/pip-req-build-1l5mv21q
  Complete output (22 lines):
  Traceback (most recent call last):
    File "/tmp/tmplww0m12f", line 280, in <module>
      main()
    File "/tmp/tmplww0m12f", line 263, in main
      json_out['return_val'] = hook(**hook_input['kwargs'])
    File "/tmp/tmplww0m12f", line 204, in build_wheel
      return _build_backend().build_wheel(wheel_directory, config_settings,
    File "/tmp/pip-build-env-q8mbkpbl/overlay/lib/python3.8/site-packages/poetry/core/masonry/api.py", line 56, in build_wheel
      return WheelBuilder.make_in(
    File "/tmp/pip-build-env-q8mbkpbl/overlay/lib/python3.8/site-packages/poetry/core/masonry/builders/wheel.py", line 85, in make_in
      wb.build(target_dir=directory)
    File "/tmp/pip-build-env-q8mbkpbl/overlay/lib/python3.8/site-packages/poetry/core/masonry/builders/wheel.py", line 121, in build
      self._copy_module(zip_file)
    File "/tmp/pip-build-env-q8mbkpbl/overlay/lib/python3.8/site-packages/poetry/core/masonry/builders/wheel.py", line 232, in _copy_module
      to_add = self.find_files_to_add()
    File "/tmp/pip-build-env-q8mbkpbl/overlay/lib/python3.8/site-packages/poetry/core/masonry/builders/builder.py", line 199, in find_files_to_add
      include_file.relative_to_project_root()
    File "/tmp/pip-build-env-q8mbkpbl/overlay/lib/python3.8/site-packages/poetry/core/masonry/builders/builder.py", line 395, in relative_to_project_root
      return self.path.relative_to(self.project_root)
    File "/usr/lib/python3.8/pathlib.py", line 908, in relative_to
      raise ValueError("{!r} does not start with {!r}"
  ValueError: '/home/cweaver/spt3g_software/cmake/init.py' does not start with '/tmp/pip-req-build-1l5mv21q'
  ----------------------------------------
  ERROR: Failed building wheel for spt3g
Failed to build spt3g
ERROR: Could not build wheels for spt3g which use PEP 517 and cannot be installed directly

$ cd
$ poetry add -e /home/cweaver/spt3g_software/build

Poetry could not find a pyproject.toml file in /home/cweaver or its parents

I've never worked with poetry before, but it seems to get confused if the command is executed from inside the build directory and decide that there is a circular dependency, gets confused if executed from another directory and can't figure out where pyproject.toml is, and has some complicated error if invoked by pip.

It seems un-idiomatic from both CMake and Python packaging persepctives to have a file named pyproject.toml which is not actually usable as-is. I suggest that this source code file should be moved to something like cmake/pyproject.toml.in so that it is out of the way and obviously only an input file from which a usable version is produced. It looks like there's also the issue here that the version should be determined dynamically, but poetry apparently does not support this without an additional plugin.

More generally, I think we want to have compatibility with PEP 517, but I'm not sure if this is how we want to do it, or whether it is really necessary for what you're trying to do. It's a bit peculiar to have a python install step which can only be run after compiling the code (I assume it only makes sense then, although I don't see anything defined in pyproject.toml which will install the compiled libraries? The poetry docs are so vague I can't find any mention of what it will pick up and install), but the dependency isn't enforced by the build system. At the same time, it seems like this has a lot of overlap with the install target, which already installs the software and has its relationship with compilation correctly enforced, though it does not manage python dependencies. For the installation portion, it seems like you would be better served by just using the existing make install (and if there's some part of that which is incomplete, we should fix it)?

As far as dependencies go, we should certainly write them down, and perhaps have an easy way to get them installed, but I'm not sure if this is the way to do it. I'm not sure if anyone else is typically using poetry on SPT, so I think we should consider carefully before making the choice to use it as the build system, for example, instead of the much more mainstream setuptools (which simonsobs/so3g uses in a way that it looks like PEP-517 could be relatively simply layered on top of), or perhaps scikit-build, which appears to be one of the more highly recommended ways to make a PEP-517 build easily drive CMake (although I have not yet tried it myself). If your goal for (python) dependencies is just to get them installed, it seems like we might start from just writing down a requirements.txt so you have something simple and easy to use, and tackle the bigger question of PEP-517 compatibility separately.

[marius311]

Thanks for the comments.

On the PEP517 side, my only goal/requirement is to make it so one can add a conventionally-built spt3g_software to a PEP517 virtual environment, because I use these alot, they're great, and my reading is that its the defacto standard in Python moving forward (poetry being one of the things compatible with them, also pipx, sorry I think I was wrong about just pip before).

There's no attempt to "use poetry as a the build system" or anything like that. I think it would be awesome to have that and get rid of some of the limitations of this approach, but thats well beyond my scope here.

Fyi, the series of commands you want for poetry is roughly:

$ ./configure && make # as usual
$ cd 
$ mkdir my_poetry_env && cd my_poetry_env
$ poetry init -n --python ^3.8
$ poetry add -e spt3g_software/build

and you'll have a virtual environment with spt3g_software and all of its Python dependencies installed. You can also install the same build folder into different environments if you want, which you can't do with spt3g_software's make install. Here's and example somewhere I'm using this, if its helpful at all.

So bottom line is I'm sympathetic to alot of what you say but I view this is a partial solution that just adds another option, and could be improved in the future.

I suggest that this source code file should be moved to something like cmake/pyproject.toml.in so that it is out of the way and obviously only an input file from which a usable version is produced

Seems reasonable, will do.

It looks like there's also the issue here that the version should be determined dynamically

Sorry don't totally follow this, as there is already code here to dynamically determine this.

[cnweaver]

the series of commands you want for poetry is roughly. . .

Okay, so what I was missing is that poetry must either be run in the venv directory or with -C $VIRTUAL_ENV. While that is somewhat awkward, it does then work. The result is a partial install of the package; python files are placed in the appropriate lib/python3.x/site_packages, but the libraries are embedded inside that (which works for python but is otherwise non-optimal), and header files and cmake config scripts are unsurprisingly missing. This is not good, because there are various downstream uses of 3G software (simonsobs/so3g, cmb-S4/collector-prototype) which require these parts to be there, and we probably shouldn't have an install method which produces installations that work only for some users? If we do choose to go such a route, this will need to be clearly documented as a known bug/incomplete feature, so that advanced users know it won't work for them.

There's no attempt to "use poetry as a the build system" or anything like that.

This patch does use poetry as the build system in that it requires any PEP-517-compliant frontend to use poetry's package building logic (this is exactly what build-backend = "poetry.core.masonry.api" entails). This is why attempting to then install the package with pip starts out fine, and would work in principle, but currently fails with an error inside of poetry. Fixing the above partial installation issue for PEP-517 installs depends on which build backend is used. Using poetry for this can be a valid choice (though I have no idea how to do a complete job of it); I just wanted to note that it is a significant choice for the project, since it requires the maintainers to commit to working with it (unless they deliberately replace it with something else), and so this probably deserves some thought.

The fact that using another PEP-517 frontend with this config file doesn't seem to work suggests to me that there's something still missing or not quite right. (At a minimum, documentation to tell me not to just run the obvious pip install command, and what to do instead if using pip!)

Sorry don't totally follow this, as there is already code here to dynamically determine [the version].

That works, but there is a common mechanism for this, dynamic = ["version"], which is supported by setuptools and flit, and scikit-build-core is adding it. This limitation makes it hard eventually make this work in the way that I've seen in various other projects where there is a valid pyproject.toml at the root of the project which can build and install the entire thing in one step, including compiling native components. This is not something I'm saying we need, just something which might be nice, and we should be aware that poetry can't (at present) do it.

[marius311]

The result is a partial install of the package; python files are placed in the appropriate lib/python3.x/site_packages, but the libraries are embedded inside that

Are you sure you did add -e? The result of that should be just a lib/python3.X/spt3g.pth file pointing to the actual directory.

Re: poetry. I see what you mean, I guess this uses poetry as the "build system" (although nothing is built, for now, thats done by hand using cmake), but it leaves the package installable by anything thats PEP517 compliant I think is the way it works? For me the following worked and installed all of spt3g_software's deps as well as linked it with a .pth file:

pip install -e ~/path/to/spt3g_software/build/

(Python 3.10.6 / pip 22.3.1)

Addressed the other comments in the recent commits.

[cnweaver]

Are you sure you did add -e? The result of that should be just a lib/python3.X/spt3g.pth file pointing to the actual directory.

Adding the -e option does do that, but that's going in the wrong direction, as it now installs nothing, and the installation depends on keeping the build directory around. Editable installs are intended to support local development of the package being installed and are not what users should generally be doing. In any case, this does not address the needs of users who need the header files and libraries properly installed.

The pip install failure seems to be due to some incompatibility between pip 20 and poetry 1.4. With pip 22 or 23 it works, although many people will not have a pip that new by default.

I think that @arahlin and @nwhitehorn should make the decision on whether an optional, partial installation method should be included, but if it is to be, I think it (and its limitations) must be documented, probably in https://github.com/CMB-S4/spt3g_software/blob/master/doc/quickstart.rst .

[arahlin]

I think that @arahlin and @nwhitehorn should make the decision on whether an optional, partial installation method should be included, but if it is to be, I think it (and its limitations) must be documented, probably in https://github.com/CMB-S4/spt3g_software/blob/master/doc/quickstart.rst .

I agree that whatever solution we have should be very clearly documented, especially in terms of its limitations. I'm not strictly opposed to this partial solution, although longer-term we really should make this package pip-installable without caveats.

[marius311]

Took a stab at docs (please feel free to edit)

[arahlin]

Based on the work in #112, I would remove the bz2 bits from this PR.

[marius311]

Addressed the comment about Python 3.7 (allowed it) and merged upstream bzip2 changes from the other PR. Ready for review from my end.

[arahlin]

I've merged the smaller cmake changes directly into master. @marius311 if you can merge those in here, this PR will then be just the poetry bits.

[nwhitehorn]

Looks good! Please retool the PR description before merging, since some pieces were previously merged.