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.

Sign up for GitHub

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

Clean up handling of Legion config options #670

Open
wants to merge 6 commits into
base: branch-24.03
Choose a base branch
from
Open

Clean up handling of Legion config options #670

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dbcfa36
Always build (and require) Legion with python support and bindings
manopapad Apr 7, 2023
e2671dd
Move unconditional Legion configuration flags out of install.py
manopapad Apr 7, 2023
1594c95
Document major required Legion configuration flags
manopapad Apr 7, 2023
bda225b
Move the legion configuration check to the right place
manopapad Apr 7, 2023
640bf97
Fix indentation
manopapad Apr 7, 2023
ee99cd1
Can check REDOP_COMPLEX and REDOP_HALF using #defines
manopapad Apr 7, 2023
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
1 change: 0 additions & 1 deletion cmake/Modules/legate_core_options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ option(Legion_USE_HDF5 "Enable support for HDF5" OFF)
option(Legion_USE_CUDA "Enable Legion support for the CUDA runtime" OFF)
option(Legion_NETWORKS "Networking backends to use (semicolon-separated)" "")
option(Legion_USE_OpenMP "Use OpenMP" OFF)
option(Legion_USE_Python "Use Python" OFF)
option(Legion_BOUNDS_CHECKS "Enable bounds checking in Legion accessors" OFF)

if("${Legion_NETWORKS}" MATCHES ".*gasnet(1|ex).*")
Expand Down
17 changes: 14 additions & 3 deletions cmake/thirdparty/get_legion.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,16 @@ function(find_or_configure_legion)

if(Legion_FOUND)
message(STATUS "CPM: using local package Legion@${version}")

# Check that required Legion options are set
if(NOT Legion_USE_Python)
message(FATAL_ERROR "Legion was not compiled with Legion_USE_Python")
endif()
Comment on lines +75 to +77
Copy link

Choose a reason for hiding this comment

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

This makes it impossible to build the legate C++ library without Legion built w/ Python bindings. Is this really what we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is my attempt at avoiding the following fell-bad scenario:

  • user builds Legion separately, forgets to add python support
  • user builds legate.core C++ bits using cmake directly
  • users does pip install
  • user realizes nothing is working, because they built Legion w/o Python support (and the build process never complained)

I would also be ok with a situation where the cmake build doesn't enforce Legion_USE_Python, and instead that becomes necessary only when you try to install the python pieces of legate.core. Do you think that's possible/preferable? Or maybe you have a different approach to avoid this scenario?


# TODO: The following should also be checked, but are not currently reported by Legion:
# Legion_BUILD_BINDINGS
# Legion_REDOP_HALF
# Legion_REDOP_COMPLEX
else()

include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules/cpm_helpers.cmake)
Expand Down Expand Up @@ -179,6 +189,7 @@ function(find_or_configure_legion)
FIND_PACKAGE_ARGUMENTS EXACT
EXCLUDE_FROM_ALL ${exclude_from_all}
OPTIONS ${_legion_cuda_options}
"BUILD_SHARED_LIBS ON"
"CMAKE_CXX_STANDARD ${_cxx_std}"
"Legion_VERSION ${version}"
"Legion_BUILD_BINDINGS ON"
Expand All @@ -195,19 +206,19 @@ function(find_or_configure_legion)
"Legion_USE_CUDA ${Legion_USE_CUDA}"
"Legion_NETWORKS ${Legion_NETWORKS}"
"Legion_USE_OpenMP ${Legion_USE_OpenMP}"
"Legion_USE_Python ${Legion_USE_Python}"
"Legion_USE_Python ON"
"Legion_BOUNDS_CHECKS ${Legion_BOUNDS_CHECKS}"
"Legion_BUILD_JUPYTER ON"
"Legion_EMBED_GASNet_CONFIGURE_ARGS --with-ibv-max-hcas=8"
)
endif()

jjwilke marked this conversation as resolved.
Show resolved Hide resolved
set(Legion_USE_CUDA ${Legion_USE_CUDA} PARENT_SCOPE)
set(Legion_USE_OpenMP ${Legion_USE_OpenMP} PARENT_SCOPE)
set(Legion_USE_Python ${Legion_USE_Python} PARENT_SCOPE)
set(Legion_NETWORKS ${Legion_NETWORKS} PARENT_SCOPE)

message(VERBOSE "Legion_USE_CUDA=${Legion_USE_CUDA}")
message(VERBOSE "Legion_USE_OpenMP=${Legion_USE_OpenMP}")
message(VERBOSE "Legion_USE_Python=${Legion_USE_Python}")
message(VERBOSE "Legion_NETWORKS=${Legion_NETWORKS}")

endfunction()
Expand Down
4 changes: 1 addition & 3 deletions conda/conda-build/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ CMAKE_ARGS="$(echo "$CMAKE_ARGS" | sed -r "s@_INCLUDE=ONLY@_INCLUDE=BOTH@g")"
CMAKE_ARGS+="
--log-level=VERBOSE
-DBUILD_MARCH=haswell
-DLegion_USE_OpenMP=ON
-DLegion_USE_Python=ON
-DLegion_BUILD_BINDINGS=ON"
-DLegion_USE_OpenMP=ON"

# We rely on an environment variable to determine if we need to build cpu-only bits
if [ -z "$CPU_ONLY" ]; then
Expand Down
18 changes: 9 additions & 9 deletions install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,11 +420,13 @@ def validate_path(path):
if debug or verbose:
cmake_flags += [f"--log-level={'DEBUG' if debug else 'VERBOSE'}"]

# NOTE: Any unconditional Legion configuration settings should be added to
# cmake/thirdparty/get_legion.cmake, so that pure-cmake builds will also
# pick them up.
cmake_flags += f"""\
-DCMAKE_BUILD_TYPE={(
"Debug" if debug else "RelWithDebInfo" if debug_release else "Release"
)}
-DBUILD_SHARED_LIBS=ON
-DBUILD_MARCH={march}
-DCMAKE_CUDA_ARCHITECTURES={arch}
-DLegion_MAX_DIM={str(maxdim)}
Expand All @@ -436,13 +438,7 @@ def validate_path(path):
-DLegion_USE_LLVM={("ON" if llvm else "OFF")}
-DLegion_NETWORKS={";".join(networks)}
-DLegion_USE_HDF5={("ON" if hdf else "OFF")}
-DLegion_USE_Python=ON
-DLegion_Python_Version={pyversion}
-DLegion_REDOP_COMPLEX=ON
jjwilke marked this conversation as resolved.
Show resolved Hide resolved
-DLegion_REDOP_HALF=ON
-DLegion_BUILD_BINDINGS=ON
-DLegion_BUILD_JUPYTER=ON
-DLegion_EMBED_GASNet_CONFIGURE_ARGS="--with-ibv-max-hcas=8"
""".splitlines()

if nccl_dir:
Expand Down Expand Up @@ -747,14 +743,18 @@ def driver():
dest="legion_dir",
required=False,
default=None,
help="Path to an existing Legion build directory.",
help="Path to an existing Legion build directory. A recent checkout "
"of the control_replication branch is required, configured with "
jjwilke marked this conversation as resolved.
Show resolved Hide resolved
"Legion_BUILD_BINDINGS=ON, Legion_REDOP_HALF=ON, "
"Legion_REDOP_COMPLEX=ON, Legion_USE_Python=ON.",
)
parser.add_argument(
"--legion-src-dir",
dest="legion_src_dir",
required=False,
default=None,
help="Path to an existing Legion source directory.",
help="Path to an existing Legion source directory. A recent checkout "
"of the control_replication branch is required.",
)
parser.add_argument(
"--legion-url",
Expand Down
18 changes: 5 additions & 13 deletions legate_core_cpp.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,9 @@ macro(_enable_cuda_language)
endif()
endmacro()

if(Legion_USE_Python)
_find_package_Python3()
if(Python3_FOUND AND Python3_VERSION)
set(Legion_Python_Version ${Python3_VERSION})
endif()
_find_package_Python3()
if(Python3_FOUND AND Python3_VERSION)
set(Legion_Python_Version ${Python3_VERSION})
endif()

if(Legion_USE_CUDA)
Expand All @@ -102,20 +100,15 @@ endif()

###
# If we find Legion already configured on the system, it will report whether it
# was compiled with Python (Legion_USE_PYTHON), CUDA (Legion_USE_CUDA), OpenMP
# (Legion_USE_OpenMP), and networking (Legion_NETWORKS).
# was compiled with CUDA (Legion_USE_CUDA), OpenMP (Legion_USE_OpenMP), and
# networking (Legion_NETWORKS).
#
# We use the same variables as Legion because we want to enable/disable each of
# these features based on how Legion was configured (it doesn't make sense to
# build legate.core's Python bindings if Legion's bindings weren't compiled).
###
include(cmake/thirdparty/get_legion.cmake)

# If Legion_USE_Python was toggled ON by find_package(Legion), find Python3
if(Legion_USE_Python AND (NOT Python3_FOUND))
_find_package_Python3()
endif()

if(Legion_NETWORKS)
find_package(MPI REQUIRED COMPONENTS CXX)
endif()
Expand Down Expand Up @@ -458,7 +451,6 @@ endif()
]=]
"set(Legion_USE_CUDA ${Legion_USE_CUDA})"
"set(Legion_USE_OpenMP ${Legion_USE_OpenMP})"
"set(Legion_USE_Python ${Legion_USE_Python})"
"set(Legion_NETWORKS ${Legion_NETWORKS})"
"set(Legion_BOUNDS_CHECKS ${Legion_BOUNDS_CHECKS})"
[=[
Expand Down
2 changes: 0 additions & 2 deletions legate_core_python.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ endif()

if(NOT legate_core_FOUND)
set(SKBUILD OFF)
set(Legion_USE_Python ON)
set(Legion_BUILD_BINDINGS ON)
add_subdirectory(. "${CMAKE_CURRENT_SOURCE_DIR}/build")
set(SKBUILD ON)
endif()
Expand Down
2 changes: 0 additions & 2 deletions scripts/build-separately-no-install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ if [[ -n "$(which ninja)" ]]; then cmake_args+=" -GNinja"; fi
cmake_args+="
-D Legion_USE_CUDA=ON
-D Legion_USE_OpenMP=ON
-D Legion_USE_Python=ON
-D Legion_BUILD_BINDINGS=ON
-D CMAKE_CUDA_ARCHITECTURES=NATIVE
";

Expand Down