[c++/python] Build flow improvements #496

gspowley · 2022-11-01T19:04:29Z

A number of build flow improvements based on developer feedback:

Document a reliable way to build tiledbsoma, including after pulling the latest source code.
Add a developer Makefile to automate build commands.
Add more documentation on components of build flow (see libtiledbsoma/README.md).
Provide a way to build only a libtiledbsoma static library for R.
Merge C++ tests into Python CI to reduce redundant CI runs.

Example C++ dev commands:

# setup python virtual env
python -m venv test/tiledbsoma
source test/tiledbsoma/bin/activate

# install developer requirements
pip install -r apis/python/requirements_dev.txt

# install and test tiledbsoma
make install test

R build command:

# requires cmake >= 3.21
make r-build

shortcut-integration · 2022-11-01T19:04:32Z

This pull request has been linked to Shortcut Story #22584: Some centralization of R/C++ and Python/C++ builds.

eddelbuettel · 2022-11-01T23:33:01Z

Looks fabulous at a first glance. Will test-drive after dinner. Love the Makefile which I find easier to read / scan / understand that the verbosity of 'that other flavor'.

johnkerl · 2022-11-01T23:32:13Z

apis/python/tests/__init__.py

@@ -1,6 +1,9 @@
 import pyarrow as pa
 from typeguard.importhook import install_import_hook

+# avoid typeguard by importing before calling install_import_hook


johnkerl · 2022-11-01T23:32:57Z

libtiledbsoma/README.md

+cd TileDB-SOMA
+```
+---
+## Setup a Python Virtual Environment


nit: Set up a ...

johnkerl · 2022-11-01T23:33:16Z

libtiledbsoma/README.md

-
-Remove old build files:
+---
+## Python Only Development


nit: Python-only Development

johnkerl · 2022-11-01T23:36:20Z

scripts/bld

-BUILD_TYPE=${1:-Release} # default to Release build
-if [ ! -z "$LIBTILEDBSOMA_DEBUG_BUILD" ]; then
-  echo
-  echo ================================================================
-  echo LIBTILEDBSOMA SCRIPTS/BLD BUILD_TYPE ${BUILD_TYPE}
-  echo ================================================================
-else
-  echo "Building ${BUILD_TYPE} build"
-fi


I found the output from this verbose setup far easier to parse -- while it lasted. :(

👋 ✌️ 🖖

I found all of the debug messages made the code harder to modify/maintain.

@gspowley which I knew would happen hence 🖖

johnkerl · 2022-11-01T23:45:41Z

Makefile

+
+# - incrementally compile and install the c++ code
+# - copy the shared objects on top of the python dev build
+.PHONY: update


Linux error:

$ make update make[1]: /tmp/pip-build-env-imfu1ozp/overlay/local/lib/python3.10/dist-packages/cmake/data/bin/cmake: No such file or directory make[1]: *** [Makefile:180: cmake_check_build_system] Error 127 make: *** [Makefile:9: update] Error 2

MacOS error:

johnkerl@Kerl-MBP[prod][gspowley/sc-22584/build-flow-improvements][TileDB-SOMA]$ make update [ 50%] Built target ep_spdlog [ 56%] Performing configure step for 'libtiledbsoma' -- Using default install prefix /Users/johnkerl/git/single-cell-data/TileDB-SOMA/dist. To control CMAKE_INSTALL_PREFIX, set OVERRIDE_INSTALL_PREFIX=OFF -- Install prefix is /Users/johnkerl/git/single-cell-data/TileDB-SOMA/dist. -- Skipping search for TileDB, building it as an external project. To use system TileDB, set FORCE_BUILD_TILEDB=OFF -- Starting TileDB-SOMA regular build. -- Performing Test HAVE_AVX2 -- Performing Test HAVE_AVX2 - Success -- Starting TileDB-SOMA build. -- Found TileDB: TILEDB_LIB-NOTFOUND -- Building with commit hash 4ba0d1e CMake Warning at src/CMakeLists.txt:57 (message): Building TileDB without deprecation warnings CMake Error at src/pyapi/CMakeLists.txt:3 (find_package): Could not find a package configuration file provided by "pybind11" with any of the following names: pybind11Config.cmake pybind11-config.cmake Add the installation prefix of "pybind11" to CMAKE_PREFIX_PATH or set "pybind11_DIR" to a directory containing one of the above files. If "pybind11" provides a separate development package or SDK, be sure it has been installed. -- Configuring incomplete, errors occurred! See also "/Users/johnkerl/git/single-cell-data/TileDB-SOMA/build/libtiledbsoma/CMakeFiles/CMakeOutput.log". See also "/Users/johnkerl/git/single-cell-data/TileDB-SOMA/build/libtiledbsoma/CMakeFiles/CMakeError.log". make[3]: *** [libtiledbsoma-prefix/src/libtiledbsoma-stamp/libtiledbsoma-configure] Error 1 make[2]: *** [CMakeFiles/libtiledbsoma.dir/all] Error 2 make[1]: *** [all] Error 2 make: *** [update] Error 2

Note this happened on MacOS even after pip install -r apis/python/requirements_dev.txt

make[1]: /tmp/pip-build-env-imfu1ozp/overlay/local/lib/python3.10/dist-packages/cmake/data/bin/cmake: No such file or directory
make[1]: *** [Makefile:180: cmake_check_build_system] Error 127
make: *** [Makefile:9: update] Error 2

The Linux error is definitely cause by running something like this (because I've seen this before):

pip install -e apis/python make update

Here's what happens:

pip install creates an overlay environment (/tmp/pip-build-env-imfu1ozp/overlay), where it install cmake and builds the code.

make update tries to call the cmake from the overlay environment, but it is no longer there.

The MacOS error may be caused by the same sequence of commands.

The libtiledbsoma/README.md describes two dev modes (Python-only and Python and C++) and which build commands should be used in each.

Maybe we should have just one developer mode that uses the Makefile and avoid the confusion?

johnkerl · 2022-11-01T23:46:39Z

Makefile

+# - run the clean rule
+# - build the c++ static library for r
+.PHONY: r-build
+r-build: clean


johnkerl · 2022-11-01T23:46:47Z

Makefile

+# - run c++ unit tests
+# - run pytests
+.PHONY: test
+test: test/soco


Makefile

eddelbuettel

The make r-build target is close in that it gets the desired static library and the core headers -- but it misses the externals/ header directory we need for span under C++17.

gspowley · 2022-11-02T13:28:50Z

The make r-build target is close in that it gets the desired static library and the core headers -- but it misses the externals/ header directory we need for span under C++17.

cmake >= 3.23 makes this easier to support. Does this install tree capture everything?

dist/
├── include
│   ├── externals
│   │   └── span
│   │       └── span.hpp
│   └── tiledbsoma
│       ├── array_buffers.h
│       ├── column_buffer.h
│       ├── common.h
│       ├── logger_public.h
│       ├── managed_query.h
│       ├── soma_reader.h
│       ├── tiledbsoma
│       ├── tiledbsoma_export.h
│       └── util.h
└── lib
    ├── libtiledb.so.2.12
    └── libtiledbsoma.a

eddelbuettel · 2022-11-02T13:31:51Z

Thanks, I noticed the change late last eve but I will need a newer cmake first to see and test. The image looks good ~~(though strictly speaking I would want just the .a library)~~. Also, can I give a DESTDIR to bld ?

Edit Sorry, it is also missing the two arrow headers we should move to a different directory. They are currently 'hiding' with the two Python headers but are not Python-specific. The R package needs them too.

Edit 2 Plus the thread_pool/ headers.

eddelbuettel · 2022-11-02T14:11:40Z

Iff the additional headers are added we can build, and link, and load. Note that in the what I quote below we now only compile the r-api source files (for now all starting with r namely rinterface.cpp, riterator.cpp and rutilities.cpp) plus the Rcpp-auto-generated glue RcppExports.cpp. We no longer compile the libtiledbsoma files, instead we link with libtiledbsoma.a.

And the R package does a full 'does it turn on?' test by trying to load at the penultimate step so this really is good.

(But I did have to add extra headers in directory thread_pool/ and pyapi. So we are close. We can see the cigar.)

edd@rob:~/git/tiledb-soma/apis/r(gspowley/sc-22584/build-flow-improvements)$ ./cleanup 
edd@rob:~/git/tiledb-soma/apis/r(gspowley/sc-22584/build-flow-improvements)$ install.r       # simple wrapper to R CMD INSTALL
* installing *source* package found in current working directory ...
* installing *source* package ‘tiledbsoma’ ...
** using staged installation
** updated src/Makevars for system library via pkg-config
** libs
ccache g++  -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG -I. -I../inst/include/ -I/usr/local/include -I/usr/local/lib/R/site-library/arch/include -DR_BUILD   -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppSpdlog/include' -I'/usr/local/lib/R/site-library/arch/include'    -fpic  -g -O3 -Wall -pipe -pedantic -Wno-ignored-attributes  -c RcppExports.cpp -o RcppExports.o
ccache g++  -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG -I. -I../inst/include/ -I/usr/local/include -I/usr/local/lib/R/site-library/arch/include -DR_BUILD   -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppSpdlog/include' -I'/usr/local/lib/R/site-library/arch/include'    -fpic  -g -O3 -Wall -pipe -pedantic -Wno-ignored-attributes  -c rinterface.cpp -o rinterface.o
ccache g++  -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG -I. -I../inst/include/ -I/usr/local/include -I/usr/local/lib/R/site-library/arch/include -DR_BUILD   -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppSpdlog/include' -I'/usr/local/lib/R/site-library/arch/include'    -fpic  -g -O3 -Wall -pipe -pedantic -Wno-ignored-attributes  -c riterator.cpp -o riterator.o
ccache g++  -std=gnu++17 -I"/usr/share/R/include" -DNDEBUG -I. -I../inst/include/ -I/usr/local/include -I/usr/local/lib/R/site-library/arch/include -DR_BUILD   -I'/usr/local/lib/R/site-library/Rcpp/include' -I'/usr/local/lib/R/site-library/RcppSpdlog/include' -I'/usr/local/lib/R/site-library/arch/include'    -fpic  -g -O3 -Wall -pipe -pedantic -Wno-ignored-attributes  -c rutilities.cpp -o rutilities.o
ccache g++ -std=gnu++17 -Wl,-S -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -flto=auto -Wl,-z,relro -o tiledbsoma.so RcppExports.o rinterface.o riterator.o rutilities.o -L/usr/local/lib -ltiledb libtiledbsoma.a -L/usr/lib/R/lib -lR
if [ "" != "Windows_NT" ] && [ `uname -s` = 'Darwin' ] && [ -f ../inst/tiledb/lib/libtiledb.dylib ] && [ -f tiledbsoma.so ]; then install_name_tool -change libz.1.dylib @rpath/libz.1.dylib ../inst/tiledb/lib/libtiledb.dylib; install_name_tool -add_rpath @loader_path/../tiledb/lib tiledbsoma.so; fi
installing to /usr/local/lib/R/site-library/00LOCK-r/00new/tiledbsoma/libs
** R
** inst
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (tiledbsoma)
edd@rob:~/git/tiledb-soma/apis/r(gspowley/sc-22584/build-flow-improvements)$

eddelbuettel

Looks good to me, I should have some time this afternoon to check again on libtiledbsoma.a use and headers.

…m:single-cell-data/TileDB-SingleCell into gspowley/sc-22584/build-flow-improvements

eddelbuettel · 2022-11-07T21:38:18Z

Gave it another two spins to ascertain that

It does not break the current use, which it passes (and which is of course what CI shows us)
It does what it is supposed to do namely provide a libtiledbsoma.a plus headers we can use in apis/r and that passes too.

So 🚢 from me! Nice work.

gspowley force-pushed the gspowley/sc-22584/build-flow-improvements branch from 895e5ee to bfdfd22 Compare November 1, 2022 20:18

gspowley mentioned this pull request Nov 1, 2022

[c++] Build libtiledbsoma static library #423

Closed

Build flow improvements

4ba0d1e

gspowley force-pushed the gspowley/sc-22584/build-flow-improvements branch from bfdfd22 to 4ba0d1e Compare November 1, 2022 22:58

gspowley marked this pull request as ready for review November 1, 2022 23:25

gspowley requested review from johnkerl, bkmartinjr, eddelbuettel and mlin November 1, 2022 23:26

johnkerl requested changes Nov 1, 2022

View reviewed changes

eddelbuettel reviewed Nov 2, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

eddelbuettel suggested changes Nov 2, 2022

View reviewed changes

johnkerl approved these changes Nov 2, 2022

View reviewed changes

address review feedback

d89e062

gspowley added 2 commits November 7, 2022 10:46

address review feedback

c93c096

Merge branch 'main' into gspowley/sc-22584/build-flow-improvements

02a4b63

gspowley force-pushed the gspowley/sc-22584/build-flow-improvements branch from d89e062 to 02a4b63 Compare November 7, 2022 15:50

update r build for new header layout

d42bf9f

eddelbuettel approved these changes Nov 7, 2022

View reviewed changes

gspowley and others added 6 commits November 7, 2022 13:29

add -DR_BUILD when building for R

eb648bd

Merge branch 'gspowley/sc-22584/build-flow-improvements' of github.co…

af1bde0

…m:single-cell-data/TileDB-SingleCell into gspowley/sc-22584/build-flow-improvements

[r] No longer include arrow_adapter.h from pyapi/

523e9cf

cmake 3.21

d1f4462

pin mypy to 0.982 in CI format check

edfbe5f

move spdlog_with_R.h into c++ tree

6731967

gspowley merged commit faa9021 into main Nov 7, 2022

gspowley deleted the gspowley/sc-22584/build-flow-improvements branch November 7, 2022 21:40

eddelbuettel mentioned this pull request Nov 7, 2022

[r] Use libtiledbsoma.a in build #510

Merged

aaronwolen mentioned this pull request Dec 8, 2022

[r] Convert arrow::large_utf8() to ascii #591

Merged

johnkerl changed the title ~~Build flow improvements~~ [c++/python] Build flow improvements May 24, 2023

ryan-williams mentioned this pull request Mar 7, 2024

[python] Typeguard not running as intended #2238

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[c++/python] Build flow improvements #496

[c++/python] Build flow improvements #496

gspowley commented Nov 1, 2022 •

edited

Loading

shortcut-integration bot commented Nov 1, 2022

eddelbuettel commented Nov 1, 2022

johnkerl Nov 1, 2022

johnkerl Nov 1, 2022

johnkerl Nov 1, 2022

johnkerl Nov 1, 2022

gspowley Nov 2, 2022

johnkerl Nov 2, 2022

johnkerl Nov 1, 2022

gspowley Nov 2, 2022

johnkerl Nov 1, 2022

johnkerl Nov 1, 2022

eddelbuettel left a comment •

edited

Loading

gspowley commented Nov 2, 2022

eddelbuettel commented Nov 2, 2022 •

edited

Loading

eddelbuettel commented Nov 2, 2022 •

edited

Loading

eddelbuettel left a comment

eddelbuettel commented Nov 7, 2022

[c++/python] Build flow improvements #496

[c++/python] Build flow improvements #496

Conversation

gspowley commented Nov 1, 2022 • edited Loading

shortcut-integration bot commented Nov 1, 2022

eddelbuettel commented Nov 1, 2022

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

eddelbuettel left a comment • edited Loading

Choose a reason for hiding this comment

gspowley commented Nov 2, 2022

eddelbuettel commented Nov 2, 2022 • edited Loading

eddelbuettel commented Nov 2, 2022 • edited Loading

eddelbuettel left a comment

Choose a reason for hiding this comment

eddelbuettel commented Nov 7, 2022

gspowley commented Nov 1, 2022 •

edited

Loading

eddelbuettel left a comment •

edited

Loading

eddelbuettel commented Nov 2, 2022 •

edited

Loading

eddelbuettel commented Nov 2, 2022 •

edited

Loading