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 6 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
4 changes: 2 additions & 2 deletions RAPIDS.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ set(rapids-cmake-version 22.06)
include(FetchContent)
FetchContent_Declare(
rapids-cmake
GIT_REPOSITORY https://github.com/rapidsai/rapids-cmake.git
GIT_TAG branch-${rapids-cmake-version}
GIT_REPOSITORY https://github.com/vyasr/rapids-cmake.git
GIT_TAG rapids-cython
)
FetchContent_GetProperties(rapids-cmake)
if(rapids-cmake_POPULATED)
Expand Down
64 changes: 64 additions & 0 deletions rapids-cmake/cython/create_cython_modules.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# =============================================================================
# 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.
# =============================================================================

#[=======================================================================[.rst:
create_cython_modules
---------------------

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

.. code-block:: cmake

create_cython_modules(<ModuleName...>)

Creates a Cython target for a module, then adds a corresponding Python
extension module. This function assumes that we are in a scikit-build managed
build and that ``find_package(Cython)`` has already been called.

TODO: Is there any way to avoid the above restriction? I don't think we'd want
to add a find_package call here, but maybe it would be OK and we could use
rapids_find_package for tracking.

``cython_modules``
The list of modules to build.

``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_base_directory``
The source directory of the project. This directory is used to compute the
relative install path, which is necessary to propertly support differently
configured installations such as installing in place vs. out of place.

#]=======================================================================]
function(create_cython_modules cython_modules linked_libraries install_base_directory)
foreach(cython_module ${cython_modules})
add_cython_target(${cython_module} CXX PY3)
add_library(${cython_module} MODULE ${cython_module})
robertmaynard marked this conversation as resolved.
Show resolved Hide resolved
python_extension_module(${cython_module})

# To avoid libraries being prefixed with "lib".
set_target_properties(${cython_module} PROPERTIES PREFIX "")
foreach(lib ${linked_libraries})
target_link_libraries(${cython_module} ${lib})
vyasr marked this conversation as resolved.
Show resolved Hide resolved
endforeach()

# Compute the install directory relative to the source and rely on installs being relative to
# the CMAKE_PREFIX_PATH for e.g. editable installs.
cmake_path(RELATIVE_PATH CMAKE_CURRENT_SOURCE_DIR BASE_DIRECTORY ${install_base_directory}
OUTPUT_VARIABLE install_dst)
install(TARGETS ${cython_module} DESTINATION ${install_dst})
endforeach(cython_module ${cython_sources})
endfunction()
53 changes: 53 additions & 0 deletions rapids-cmake/cython/skbuild_patches.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# =============================================================================
# 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.
# =============================================================================

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

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

#]=======================================================================]
# TODO: Should we guard this based on a scikit-build version? Override this function to avoid
vyasr marked this conversation as resolved.
Show resolved Hide resolved
# scikit-build clobbering symbol visibility.
function(_set_python_extension_symbol_visibility _target)
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")
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()
33 changes: 33 additions & 0 deletions rapids-cmake/rapids-cython.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#=============================================================================
# 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)

find_package(PythonExtensions REQUIRED)
vyasr marked this conversation as resolved.
Show resolved Hide resolved
find_package(Cython REQUIRED)

# TODO: Verify that the scopes of the below variables (CYTHON_FLAGS,
# ignored_variable) are suitable for the way that they're being used.

# Set standard Cython directives that all RAPIDS projects should use in compilation.
set(CYTHON_FLAGS
"--directive binding=True,embedsignature=True,always_allow_keywords=True"
CACHE STRING "The directives for Cython compilation.")

# Ignore unused variable warning.
set(ignored_variable "${SKBUILD}" PARENT_SCOPE)

include(${CMAKE_CURRENT_LIST_DIR}/cython/skbuild_patches.cmake)
include(${CMAKE_CURRENT_LIST_DIR}/cython/create_cython_modules.cmake)