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

Add rapids-cython component for scikit-build based Python package builds #184

Merged
merged 52 commits into from
May 6, 2022
Merged
Show file tree
Hide file tree
Changes from 51 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
1bbb20d
Repoint fetching rapids-cmake to my fork.
vyasr Apr 27, 2022
a151e59
Add CMake file for creating Cython modules.
vyasr Apr 27, 2022
764a5cb
Fix branch name.
vyasr Apr 27, 2022
f6affe9
Correct included file name.
vyasr Apr 27, 2022
fbeb64f
Add function patching scikit-build's symbol clobbering behavior.
vyasr Apr 27, 2022
a8ef734
Add some more of the boilerplate required by all RAPIDS libraries.
vyasr Apr 27, 2022
31b9a56
Add versionadded tags.
vyasr Apr 28, 2022
d52ce7b
Move all initialization logic into a separate macro and use it to ver…
vyasr Apr 28, 2022
0187923
Update gitignore for vim.
vyasr Apr 28, 2022
408eec4
Document result variables from rapids_cython_init.
vyasr Apr 28, 2022
5c36ff1
Rename function.
vyasr Apr 28, 2022
dd0c516
Move skbuild patches to a detail namespace.
vyasr Apr 28, 2022
9ce875c
Add TODO.
vyasr Apr 28, 2022
758d5c4
Update argument parsing.
vyasr Apr 28, 2022
abcd1c7
Fix path to skbuild_patches.
vyasr Apr 28, 2022
5db75f6
Fix as many linter issues as possible.
vyasr Apr 29, 2022
8e3ebfc
Update format file with signature.
vyasr Apr 29, 2022
5fcfe16
Fix typo.
vyasr Apr 29, 2022
d5d5a26
First set of PR comments.
vyasr May 2, 2022
7afd4cb
Fix typo.
vyasr May 2, 2022
dd3731b
Fix includes.
vyasr May 2, 2022
7e86599
Add include guards.
vyasr May 2, 2022
a881122
Fix path to init from skbuild_patches.
vyasr May 2, 2022
e90df81
Move verify_init to a detail API.
vyasr May 2, 2022
e983f56
Update cmake-format file.
vyasr May 2, 2022
7fbf4af
Apply cmake-format.
vyasr May 2, 2022
cedd5a7
Rewrite create_modules in terms of filenames rather than module names.
vyasr May 2, 2022
3bdbb71
Perform proper path normalization.
vyasr May 3, 2022
7c7c48e
Create testing folder.
vyasr May 3, 2022
7c3e306
Build rapids-cython tests.
vyasr May 3, 2022
85b6c75
Add tests of init.
vyasr May 3, 2022
3fa62cf
Minimal changes to get everything working again.
vyasr May 3, 2022
6bebf21
Address most remaining PR comments.
vyasr May 4, 2022
27eb826
Get working cache of scikit-build resources and add more tests.
vyasr May 4, 2022
fb4b982
Apply formatting.
vyasr May 4, 2022
d33eb46
Fix includes.
vyasr May 4, 2022
40fb705
Make changes for CYTHON_FLAGS, show where tests still fail.
vyasr May 4, 2022
e156bc1
Move Cython tests inside download block.
vyasr May 4, 2022
5d55ac0
Style fixes.
vyasr May 4, 2022
dcb181e
Remove debug prints.
vyasr May 4, 2022
26356ea
Add comment about flags issue.
vyasr May 4, 2022
fef83a1
Fixes for CYTHON_FLAGS and tests.
vyasr May 4, 2022
4d2663a
Fixes for the install dir.
vyasr May 4, 2022
c94a750
Update module name sanitization.
vyasr May 4, 2022
2e368f9
Fix style.
vyasr May 4, 2022
7365e28
Add proper test of module creation.
vyasr May 4, 2022
69e5ecf
Add more tests.
vyasr May 4, 2022
cf54c7d
Fix up tests.
vyasr May 4, 2022
6070d9c
Revert changes to RAPIDS.cmake.
vyasr May 4, 2022
c60e921
Move include(skbuild_patches) after find_package(Cython) so that it a…
vyasr May 4, 2022
f2a780d
Update format file and example invocation for rapids_cython_create_mo…
vyasr May 5, 2022
129552a
Address final TODOs.
vyasr May 5, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,6 @@ docs/_build

## VSCode IDE
.vscode

# vim
*.sw[a-z]
15 changes: 15 additions & 0 deletions cmake-format-rapids-cmake.json
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,21 @@
"GLOBAL_TARGETS": "+",
"FIND_ARGS": "+"
}
},
vyasr marked this conversation as resolved.
Show resolved Hide resolved
"rapids_cython_init": {
"pargs": {
"nargs": "0"
}
},
"rapids_cython_create_modules": {
"pargs": {
"nargs": "0"
},
"kwargs": {
"SOURCE_FILES": "*",
"LINKED_LIBRARIES": "*",
"INSTALL_DIR": "1"
}
}

}
Expand Down
96 changes: 96 additions & 0 deletions rapids-cmake/cython/create_modules.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
# =============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software distributed under the License
# is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
# or implied. See the License for the specific language governing permissions and limitations under
# the License.
# =============================================================================

include_guard(GLOBAL)

#[=======================================================================[.rst:
rapids_cython_create_modules
----------------------------

.. versionadded:: v22.06.00

Generate C(++) from Cython and create Python modules.

.. code-block:: cmake

rapids_cython_create_modules([CXX] [SOURCE_FILES <src1> <src2> ...] [LINKED_LIBRARIES <lib1> <lib2> ... ] [INSTALL_DIR <install_path> )

Creates a Cython target for a module, then adds a corresponding Python
extension module.

.. note::
Requires :cmake:command:`rapids_cython_init` to be called before usage.

``CXX``
Flag indicating that the Cython files need to generate C++ rather than C.

``SOURCE_FILES``
The list of Cython source files to be built into Python extension modules.
Note that this function assumes that Cython source files have a one-one
correspondence with extension modules to build, i.e. for every `<Name>.pyx`
in SOURCE_FILES we assume that `<Name>.pyx` is a Cython source file for which
an extension module `<Name>` should be built.

``LINKED_LIBRARIES``
The list of libraries that need to be linked into all modules. In RAPIDS,
this list usually contains (at minimum) the corresponding C++ libraries.

``INSTALL_DIR``
The path relative to the installation prefix so that it can be converted to
an absolute path in a relocatable way. If not provided, defaults to the path
to CMAKE_CURRENT_SOURCE_DIR relative to PROJECT_SOURCE_DIR.

#]=======================================================================]
function(rapids_cython_create_modules)
vyasr marked this conversation as resolved.
Show resolved Hide resolved
include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/detail/verify_init.cmake)
rapids_cython_verify_init()
vyasr marked this conversation as resolved.
Show resolved Hide resolved

list(APPEND CMAKE_MESSAGE_CONTEXT "rapids.cython.create_modules")

set(_rapids_cython_options CXX)
set(_rapids_cython_one_value INSTALL_DIR)
set(_rapids_cython_multi_value SOURCE_FILES LINKED_LIBRARIES)
cmake_parse_arguments(RAPIDS_CYTHON "${_rapids_cython_options}" "${_rapids_cython_one_value}"
"${_rapids_cython_multi_value}" ${ARGN})

set(language "C")
if(RAPIDS_CYTHON_CXX)
set(language "CXX")
endif()

foreach(cython_filename IN LISTS RAPIDS_CYTHON_SOURCE_FILES)
# Generate a reasonable module name.
cmake_path(GET cython_filename FILENAME cython_module)
cmake_path(REMOVE_EXTENSION cython_module)

add_cython_target(${cython_module} "${cython_filename}" ${language} PY3 OUTPUT_VAR
cythonized_file)
add_library(${cython_module} MODULE ${cythonized_file})
python_extension_module(${cython_module})

# To avoid libraries being prefixed with "lib".
set_target_properties(${cython_module} PROPERTIES PREFIX "")
if(DEFINED RAPIDS_CYTHON_LINKED_LIBRARIES)
target_link_libraries(${cython_module} ${RAPIDS_CYTHON_LINKED_LIBRARIES})
endif()

# Compute the install directory relative to the source and rely on installs being relative to
# the CMAKE_PREFIX_PATH for e.g. editable installs.
if(NOT DEFINED RAPIDS_CYTHON_INSTALL_DIR)
cmake_path(RELATIVE_PATH CMAKE_CURRENT_SOURCE_DIR BASE_DIRECTORY "${PROJECT_SOURCE_DIR}"
OUTPUT_VARIABLE RAPIDS_CYTHON_INSTALL_DIR)
endif()
install(TARGETS ${cython_module} DESTINATION ${RAPIDS_CYTHON_INSTALL_DIR})
endforeach()
endfunction()
55 changes: 55 additions & 0 deletions rapids-cmake/cython/detail/skbuild_patches.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# =============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software distributed under the License
# is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
# or implied. See the License for the specific language governing permissions and limitations under
# the License.
# =============================================================================

include_guard(GLOBAL)

#[=======================================================================[.rst:
_set_python_extension_symbol_visibility
---------------------------------------

.. versionadded:: v22.06.00

The original version of this function in scikit-build runs a linker script to
modify the visibility of symbols. This version is a patch to avoid overwriting
the visibility of symbols because otherwise any library that exports symbols
with external linkage will have the visibility of those symbols changed
undesirably. We can remove this function once this issue is resolved in
scikit-build.

Issue: https://github.com/scikit-build/scikit-build/issues/668
PR: https://github.com/scikit-build/scikit-build/pull/703

#]=======================================================================]
function(_set_python_extension_symbol_visibility _target)
# TODO: Should we guard this function's existence based on a scikit-build version?
vyasr marked this conversation as resolved.
Show resolved Hide resolved
include(${CMAKE_CURRENT_FUNCTION_LIST_DIR}/verify_init.cmake)
rapids_cython_verify_init()

if(PYTHON_VERSION_MAJOR VERSION_GREATER 2)
set(_modinit_prefix "PyInit_")
else()
set(_modinit_prefix "init")
endif()
message("_modinit_prefix:${_modinit_prefix}")
if("${CMAKE_C_COMPILER_ID}" STREQUAL "MSVC")
set_target_properties(${_target} PROPERTIES LINK_FLAGS "/EXPORT:${_modinit_prefix}${_target}")
elseif("${CMAKE_C_COMPILER_ID}" STREQUAL "GNU" AND NOT ${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic looks really fragile as it maps the presumption of ld linker scripts to gcc, which seems like it will cause scikitbuild issues if they want clang / ibmxl / intel compiler support.

Anyway since we only care about gcc and clang I think we can expand the check to be
elseif(CMAKE_HOST_UNIX AND NOT ${CMAKE_SYSTEM_NAME} MATCHES "Darwin" )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to put much effort into patching this function, or should we be upstreaming these questions to scikit-build/scikit-build#703?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should push these upstream and just match the same logic as upstream here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll make a PR upstream and tag you. Leaving this thread open as a reminder that this is a task for me.

set(_script_path ${CMAKE_CURRENT_BINARY_DIR}/CMakeFiles/${_target}-version-script.map)
file(WRITE ${_script_path} # Note: The change is to this script, which does not indiscriminately
# mark all non PyInit symbols as local.
"{global: ${_modinit_prefix}${_target}; };")
set_property(TARGET ${_target} APPEND_STRING
PROPERTY LINK_FLAGS " -Wl,--version-script=\"${_script_path}\"")
endif()
endfunction()
34 changes: 34 additions & 0 deletions rapids-cmake/cython/detail/verify_init.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# =============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software distributed under the License
# is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
# or implied. See the License for the specific language governing permissions and limitations under
# the License.
# =============================================================================

include_guard(GLOBAL)

#[=======================================================================[.rst:
rapids_cython_verify_init
-------------------------

.. versionadded:: v22.06.00

Simple helper function for rapids-cython components to verify that rapids_cython_init has been called before they proceed.

.. code-block:: cmake

rapids_cython_verify_init()

#]=======================================================================]
function(rapids_cython_verify_init)
if(NOT DEFINED RAPIDS_CYTHON_INITIALIZED)
message(FATAL_ERROR "You must call rapids_cython_init before calling this function")
endif()
endfunction()
62 changes: 62 additions & 0 deletions rapids-cmake/cython/init.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
# =============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software distributed under the License
# is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
# or implied. See the License for the specific language governing permissions and limitations under
# the License.
# =============================================================================

include_guard(GLOBAL)

#[=======================================================================[.rst:
rapids_cython_init
------------------

.. versionadded:: v22.06.00

Perform standard initialization of any CMake build using scikit-build to create Python extension modules with Cython.

.. code-block:: cmake

rapids_cython_init()

Result Variables
^^^^^^^^^^^^^^^^
:cmake:variable:`RAPIDS_CYTHON_INITIALIZED` will be set to TRUE.
:cmake:variable:`CYTHON_FLAGS` will be set to a standard set of a flags to pass to the command line cython invocation.

#]=======================================================================]
macro(rapids_cython_init)
vyasr marked this conversation as resolved.
Show resolved Hide resolved
vyasr marked this conversation as resolved.
Show resolved Hide resolved
list(APPEND CMAKE_MESSAGE_CONTEXT "rapids.cython.init")
# Only initialize once.
if(NOT DEFINED RAPIDS_CYTHON_INITIALIZED)
# Verify that we are using scikit-build.
if(NOT DEFINED SKBUILD)
message(WARNING "rapids-cython expects scikit-build to be active before being used. \
The SKBUILD variable is not currently set, indicating that scikit-build \
is not active, so builds may behave unexpectedly.")
else()
# Access the variable to avoid unused variable warnings."
message(TRACE "Accessing SKBUILD variable ${SKBUILD}")
endif()

find_package(PythonExtensions REQUIRED)
find_package(Cython REQUIRED)

# Incorporate scikit-build patches.
include("${rapids-cmake-dir}/cython/detail/skbuild_patches.cmake")

if(NOT CYTHON_FLAGS)
set(CYTHON_FLAGS "--directive binding=True,embedsignature=True,always_allow_keywords=True")
endif()

# Flag
set(RAPIDS_CYTHON_INITIALIZED TRUE)
endif()
endmacro()
19 changes: 19 additions & 0 deletions rapids-cmake/rapids-cython.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
#=============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#=============================================================================
include_guard(GLOBAL)

include(${CMAKE_CURRENT_LIST_DIR}/cython/init.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/cython/create_modules.cmake)
1 change: 1 addition & 0 deletions testing/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ if(RAPIDS_CMAKE_ENABLE_DOWNLOAD_TESTS)
setup_cpm_cache()

add_subdirectory(cpm)
add_subdirectory(cython)
add_subdirectory(other)
endif()

23 changes: 23 additions & 0 deletions testing/cython/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#=============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#=============================================================================

add_cmake_config_test(rapids-cython.cmake)

add_cmake_config_test(init.cmake)
add_cmake_config_test(create_modules_errors.cmake SHOULD_FAIL "You must call rapids_cython_init before calling this function")

add_cmake_config_test(create_modules)
add_cmake_config_test(create_modules_with_library)
50 changes: 50 additions & 0 deletions testing/cython/create_modules/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#=============================================================================
# Copyright (c) 2022, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#=============================================================================

# TODO: For some reason I have to specify the minimum required cmake version
# before including init.cmake below even though none of them execute code.
vyasr marked this conversation as resolved.
Show resolved Hide resolved
# Without that ordering, I get an error when I call rapids_cython_init about
# CMP0057 violations, a policy that was introduced in 3.3. Why does it matter
# at include time instead of at run time? I assume it has something to do with
# state saved when the macro in init.cmake is defined because UseCython.cmake
# is only actually loaded when rapids_cython_init is called.

include(${rapids-cmake-dir}/cython/create_modules.cmake)
cmake_minimum_required(VERSION 3.20)
include(${rapids-cmake-dir}/cython/init.cmake)

project(rapids_cython-create_modules LANGUAGES C CXX)

# TODO: Is there any way to handle setting SKBUILD and the CMAKE_MODULE_PATH
vyasr marked this conversation as resolved.
Show resolved Hide resolved
# using a general macro defined in testing/cython/CMakeLists.txt to wrap all
# tests?
# Silence warning about running without scikit-build.
set(SKBUILD ON)

# Ensure that scikit-build's CMake files are discoverable. The glob is to
# capture the current git commit hash.
file(GLOB skbuild_resource_dir LIST_DIRECTORIES ON "${CPM_SOURCE_CACHE}/skbuild/*/skbuild/resources/cmake")
LIST(APPEND CMAKE_MODULE_PATH "${skbuild_resource_dir}")

rapids_cython_init()

# Test that an empty invocation works.
rapids_cython_create_modules()

# Test that a basic invocation with a single file works.
rapids_cython_create_modules(
SOURCE_FILES test.pyx
)
Empty file.
Loading