Update python build system #701

gspowley · 2024-04-12T12:55:04Z

The goal of this PR is to update the python build system to the more modern scikit-build-core, with inspiration from TileDB-Inc/TileDB-Vector-Search.

Looking for feedback from build experts, especially related to the current Windows failure and releasing to pypi in the future.

jdblischak · 2024-04-12T19:04:25Z

related to the current Windows failure

I think the Windows failure is due to the loss of the flag --libtiledbvcf. The Windows build setup is similar to that used by tiledb-vcf-feedstock. The C++ library is installed into the conda environment, and then the Python client finds it via --libtiledbvcf (note that in the feedstock the linux and osx builds also use this strategy).

TileDB-Vector-Search and tiledb-vector-search-feedstock are different. The recipe does not have a separate output for the C++ library. Since the C++ library is always vendored with the Python package, there is no need to have a command line flag to help the Python package find it.

TileDB-VCF is more similar to TileDB-SOMA, and it is still using setup.py. In additional to passing command line flags to setup.py, it also supports setting env vars so that you can use pip install.

I don't know if we have an example Python package that uses scikit-build-core but also maintains the feature of finding an existing shared library. @dudoslav and @ihnorton will have to advise on that aspect.

jdblischak · 2024-04-12T19:12:38Z

Also note that the macOS Python build works without --libtiledbvcf only because I put libtiledbvcf in dist/lib, which is exactly where CMake looks for it.

TileDB-VCF/.github/workflows/macos.yml

Lines 75 to 79 in 23565d2

    
           - name: Download libtiledbvcf artifact 
        
             uses: actions/download-artifact@v3 
        
             with: 
        
               name: libtiledbvcf 
        
               path: dist

Also, as an aside, I see that this macOS job suffers from the same SIP problems I just fixed yesterday in single-cell-data/TileDB-SOMA#2435. That is why DYLD_LIBRARY_PATH is empty. I'll address that in a separate PR later.

ihnorton

🎉. One high-level comment: we should probably make the libtiledbvcf cmake build do a configured install that can be found with find_package instead of the individual find_library calls for libtiledbvcf. Probably not necessary to get this PR working, but if we don't do it here then @dudoslav could take a look as a backlog item.

ihnorton · 2024-04-12T20:41:47Z

apis/python/CMakeLists.txt

+find_library(
+    TILEDBVCF_LIB
+    NAMES tiledbvcf
+    PATHS "${CMAKE_CURRENT_SOURCE_DIR}/../../dist/lib"


Not sure if this is the direct cause of the error, but this will almost certainly be /bin on windows. I believe if you remove /lib then find_library will search both on Windows.

Oh, hmm maybe not IIUC this override

TileDB-VCF/libtiledbvcf/src/CMakeLists.txt

Line 335 in 23565d2

LIBRARY DESTINATION lib

Made windows happy with proper path manipulation, which could move to an improved libtiledbvcf cmake structure.

ihnorton · 2024-04-12T20:42:48Z

apis/python/CMakeLists.txt

+        PATHS "${CMAKE_CURRENT_SOURCE_DIR}/../../dist/lib"
+    )
+endif()
+


Should probably check TILEDBVCF_LIB-FOUND here to make sure one of the statements succeeded.

Added code to check for success. Moving to find_package would definitely simplify this CMakeLists.txt file.

gspowley · 2024-04-13T22:26:38Z

I think the Windows failure is due to the loss of the flag --libtiledbvcf.

Thanks @jdblischak, this was the problem. We can now pass in the location of libtiledbvcf with the LIBTILEDBVCF_DIR environment variable.

we should probably make the libtiledbvcf cmake build do a configured install that can be found with find_package instead of the individual find_library calls for libtiledbvcf.

I agree @ihnorton. This PR works for now and @dudoslav could improve it in the future to be more idiomatic.

dudoslav

Just some minor nit-picks and questions. Before merging please:

Create sdist and wheel packages
For each of these, install them and print out their version (something like print(tilevcf.version) if it exists)
Optional: Also test on Windows, because I remember issues with it in TileDB-Vector-Search

dudoslav · 2024-04-15T07:13:02Z

apis/python/pyproject.toml

+    "Programming Language :: Python :: 3.10",
+    "Programming Language :: Python :: 3.11",
+]
+


I see that this pyproject.toml is missing dependencies, however, even the setup.py does not have any requires (except the build_requires). Doesn't this project really depend on any other packages? (tiledb, tiledb-cloud, numpy, etc?)

This has been a long-standing frustration of mine since it makes it difficult to know what needs to be installed (eg for nightly builds or conda recipes), eg most recently in #669.

I think this overhaul of the Python build system is a good time to explicitly list the required and optional dependencies

Agreed. Since VCF has been a conda only install so far, we've push dependency management to the conda environments. We will make the dependencies clear in pyproject.toml.

dudoslav · 2024-04-15T07:14:23Z

apis/python/pyproject.toml

+build-dir = "build/{wheel_tag}"
+metadata.version.provider = "scikit_build_core.metadata.setuptools_scm"
+sdist.include = ["src/tiledbvcf/version.py"]
+sdist.cmake = true


I don't think that this is needed. This just runs CMake during sdist packaging process. In Vector-Search we use this to generate version files/query git status, since .git is not present in sdist package.

Ok, we can remove this.

dudoslav · 2024-04-15T07:17:27Z

apis/python/CMakeLists.txt

+cmake_minimum_required(VERSION 3.21)
+
+project(
+    ${SKBUILD_PROJECT_NAME}


I prefer not to use SKBUILD_PROJECT_NAME because it implies that we are going to run this CMakeLists.txt only in scikit-build-core environment. Same goes for the version. But this is just a suggestion, if you think that no-one will ever call this cmake file on its own then keep it as it is.

I expect this CMakeLists.txt will only be called in the scikit-build-core environment. Let's keep it as.

dudoslav · 2024-04-15T07:19:08Z

apis/python/CMakeLists.txt

+message(STATUS "Searching for libtiledbvcf in ${LIBTILEDBVCF_SEARCH_PATHS}")
+
+# TODO: Use find_package() instead of find_library() to find libtiledbvcf.
+find_library(


Can't we use find_package? Or if this is not a different package but build by this project then add_subdirectory?

Yes, that was @ihnorton's comment too. The libtiledbvcf cmake files will need some work to support a different approach. I'll add that task as a backlog item instead of blocking this PR.

dudoslav · 2024-04-15T07:22:30Z

apis/python/CMakeLists.txt

+
+target_include_directories(
+    ${VCF_TARGET_NAME} PRIVATE
+    ${pybind11_INCLUDE_DIR}


This should not be needed. PYBIND headers are included automatically by pybind11_add_module.

dudoslav · 2024-04-15T07:25:32Z

apis/python/build-wheels.sh

@@ -0,0 +1,19 @@
+#!/usr/bin/env bash


Can we use cibuildwheel instead of this script?

Good suggestion. These are VCF's first steps towards building wheels and suggestions for standard processes are welcome.

dudoslav · 2024-04-15T07:26:24Z

apis/python/requirements-dev.txt

@@ -0,0 +1,7 @@
+black


What is the difference between the dev requirements and requirements for the package? (This is genuine question I don't know :D )

Similar to my comment above (https://github.com/TileDB-Inc/TileDB-VCF/pull/701/files#r1565992899), I agree that this file requirements-dev.txt is confusing because it includes more than just development-only dependencies. Here is how I would classify them based on my current understanding:

dev (only needed for local development and CI): black

runtime (required to import tiledbvcf because they are hit by __init__.py): pandas, pyarrow, pyarrow-hotfix

optional (conditionally used if installed): dask

test (required to run the test suite): dask, pytest, tiledb

The requirements-dev.txt was only there for building a local dev environment. As mentioned above, we'll make the dependencies clear in a more standard way.

jdblischak · 2024-04-15T15:11:22Z

The nightly builds against libtiledb 2.20 are failing with the CMake 2.29.1 error. @gspowley please rebase your PR onto main to incorporate 23565d2, (#700), which bumps the nightlies to build against libtiledb 2.21 (which had the fix backported)

jdblischak · 2024-04-15T15:19:15Z

this was the problem

Great! Glad this was able to fix the Windows build

We can now pass in the location of libtiledbvcf with the LIBTILEDBVCF_DIR environment variable.

@gspowley how strong is your preference for LIBTILEDBVCF_DIR over the existing LIBTILEDBVCF_PATH env var? Are you following a convention for these types of env vars? I would default to keeping the env var name the same for two primary motivations:

Avoid breaking any existing setups. Currently users can install tiledbvcf-py with pip install plus LIBTILEDBVCF_PATH. If the env var name is kept the same, the transition to scikit-build-core would be seamless
Consistency with TileDB-SOMA. The setup.py for tiledbsoma-py uses the env vars TILEDB_PATH and TILEDBSOMA_PATH

jdblischak · 2024-04-15T18:05:19Z

I tested this PR in my fork of tiledb-vcf-feedstock.

Note that the failing win-64 build is a known issue due to linking to libfmt (discussed in TileDB-Inc/tiledb-vcf-feedstock#116). The error messages are the same as in the failing CI for those most recently merged commit, TileDB-Inc/tiledb-vcf-feedstock@761620c

gspowley · 2024-04-15T18:12:55Z

how strong is your preference for LIBTILEDBVCF_DIR over the existing LIBTILEDBVCF_PATH

@jdblischak not very strong. I was pattern matching the environment variables for finding tiledb, htslib, spdlog, catch, and cli11. I value not breaking things over matching names, so we can go back to LIBTILEDBVCF_PATH.

…; fix windws install

jdblischak · 2024-04-16T14:51:17Z

I tested the most recent changes in the feedstock. The linux and osx builds are now failing with the following configuration error that has something to do with aws-c-auth. Note that I replaced the long environment path with $CONDA_PREFIX to improve readability:

https://dev.azure.com/jdblischak/feedstock-builds/_build/results?buildId=744&view=logs&j=656edd35-690f-5c53-9ba3-09c10d0bea97&t=986b1512-c876-5f92-0d81-ba851554a0a3

  -- Found PythonInterp: $CONDA_PREFIX/bin/python (found suitable version "3.10.14", minimum required is "3.6")
  -- Found PythonLibs: $CONDA_PREFIX/lib/libpython3.10.so
  -- Performing Test HAS_FLTO
  -- Performing Test HAS_FLTO - Success
  -- Found pybind11: $CONDA_PREFIX/lib/python3.10/site-packages/pybind11/include (found version "2.12.0")
  -- Searching for libtiledbvcf in /home/conda/feedstock_root/build_artifacts/tiledb-vcf_1713276872393/work/apis/python/../../dist/lib;
  -- Found libtiledbvcf: $CONDA_PREFIX/lib/libtiledbvcf.so
  CMake Error at CMakeLists.txt:122 (install):
    install FILES given directory
    "$CONDA_PREFIX/lib/aws-c-auth"
    to install.


  -- Configuring incomplete, errors occurred!

  *** CMake configuration failed

Also, today I noticed some CMake warnings. These were there previously, so they aren't fatal, but they might be worth addressing:

  Building wheel for tiledbvcf (pyproject.toml): started
  Running command Building wheel for tiledbvcf (pyproject.toml)
  *** scikit-build-core 0.8.2 using CMake 3.29.2 (wheel)
  *** Configuring CMake...
  2024-04-16 14:26:58,194 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_BUILD_TYPE=Release
  2024-04-16 14:26:58,194 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_INSTALL_PREFIX=$CONDA_PREFIX
  2024-04-16 14:26:58,194 - scikit_build_core - WARNING - libdir/ldlibrary: $CONDA_PREFIX/lib/libpython3.10.a is not a real file!
  2024-04-16 14:26:58,194 - scikit_build_core - WARNING - Can't find a Python library, got libdir=$CONDA_PREFIX/lib, ldlibrary=libpython3.10.a, multiarch=x86_64-linux-gnu, masd=None
  2024-04-16 14:26:58,348 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_BUILD_TYPE=Release
  2024-04-16 14:26:58,348 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_INSTALL_PREFIX=$CONDA_PREFIX
  loading initial cache file build/cp310-cp310-manylinux_2_17_x86_64/CMakeInit.txt

gspowley · 2024-04-16T15:40:18Z

The linux and osx builds are now failing with the following configuration error

Thanks for the heads up. I'll follow up with a PR to fix this later today.

gspowley added 3 commits April 12, 2024 08:53

Update python build system

938ad2e

build libtiledbvcf from pip install

1629240

fix for htslib

b4fa780

gspowley requested review from ihnorton, jdblischak and dudoslav April 12, 2024 17:19

ihnorton reviewed Apr 12, 2024

View reviewed changes

gspowley added 6 commits April 13, 2024 11:35

experiment

de23435

debug

c02d67d

fix include path for external build

cb22ca5

fix include path for windows

31275eb

fix windows cli tool install

cf44831

come on windows

49900e5

gspowley marked this pull request as ready for review April 13, 2024 22:26

gspowley requested a review from awenocur April 13, 2024 22:26

awenocur approved these changes Apr 13, 2024

View reviewed changes

set LIBTILEDBVCF_DIR; reduce build and test noise

8bed3e4

dudoslav reviewed Apr 15, 2024

View reviewed changes

gspowley added 2 commits April 15, 2024 15:52

review feedback; move dependencies to pyproject.toml

6739bfd

restore dependencies for tiledb-py build; use python 3.10 in azure ci…

c5c2a28

…; fix windws install

gspowley merged commit d7bc780 into main Apr 16, 2024
23 checks passed

gspowley deleted the gspowley/update-python-build branch April 16, 2024 13:57

gspowley mentioned this pull request Apr 17, 2024

Fix python cmake issue #702

Merged

jdblischak mentioned this pull request Apr 17, 2024

The centralized nightlies job failed on Tuesday (2024-04-16) TileDB-Inc/centralized-tiledb-nightlies#3

Closed

jdblischak mentioned this pull request May 1, 2024

The new scikit-build-core setup copies external shared objects into Python wheel #717

Open

jdblischak mentioned this pull request May 9, 2024

Update to 0.32, which uses scikit-build-core TileDB-Inc/tiledb-vcf-feedstock#120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update python build system #701

Update python build system #701

gspowley commented Apr 12, 2024 •

edited

Loading

jdblischak commented Apr 12, 2024

jdblischak commented Apr 12, 2024

ihnorton left a comment

ihnorton Apr 12, 2024

ihnorton Apr 12, 2024

gspowley Apr 15, 2024

ihnorton Apr 12, 2024

gspowley Apr 15, 2024

gspowley commented Apr 13, 2024

dudoslav left a comment

dudoslav Apr 15, 2024

jdblischak Apr 15, 2024

gspowley Apr 15, 2024

dudoslav Apr 15, 2024

gspowley Apr 15, 2024

dudoslav Apr 15, 2024

gspowley Apr 15, 2024

dudoslav Apr 15, 2024

gspowley Apr 15, 2024

dudoslav Apr 15, 2024

dudoslav Apr 15, 2024

gspowley Apr 15, 2024

dudoslav Apr 15, 2024

jdblischak Apr 15, 2024

gspowley Apr 15, 2024

jdblischak commented Apr 15, 2024

jdblischak commented Apr 15, 2024

jdblischak commented Apr 15, 2024

gspowley commented Apr 15, 2024

jdblischak commented Apr 16, 2024

gspowley commented Apr 16, 2024

Update python build system #701

Update python build system #701

Conversation

gspowley commented Apr 12, 2024 • edited Loading

jdblischak commented Apr 12, 2024

jdblischak commented Apr 12, 2024

ihnorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gspowley commented Apr 13, 2024

dudoslav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdblischak commented Apr 15, 2024

jdblischak commented Apr 15, 2024

jdblischak commented Apr 15, 2024

gspowley commented Apr 15, 2024

jdblischak commented Apr 16, 2024

gspowley commented Apr 16, 2024

gspowley commented Apr 12, 2024 •

edited

Loading