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

Replace conda with pyenv to fix incorrect libstdc++ use in jammy CI. #6966

Merged
merged 6 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions 3rdparty/find_dependencies.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1338,6 +1338,7 @@ if(BUILD_GUI)
if (CPP_LIBRARY AND CPPABI_LIBRARY)
set(CLANG_LIBDIR ${llvm_lib_dir})
message(STATUS "CLANG_LIBDIR found in ubuntu-default: ${CLANG_LIBDIR}")
set(LIBCPP_VERSION ${llvm_ver})
break()
endif()
endforeach()
Expand All @@ -1362,7 +1363,10 @@ if(BUILD_GUI)
llvm-8/lib
llvm-7/lib
)
file(REAL_PATH ${CPPABI_LIBRARY} CPPABI_LIBRARY)
get_filename_component(CLANG_LIBDIR ${CPPABI_LIBRARY} DIRECTORY)
string(REGEX MATCH "llvm-([0-9]+)/lib" _ ${CLANG_LIBDIR})
set(LIBCPP_VERSION ${CMAKE_MATCH_1})
endif()

# Find clang libraries at the exact path ${CLANG_LIBDIR}.
Expand All @@ -1378,6 +1382,11 @@ if(BUILD_GUI)
target_link_libraries(3rdparty_filament INTERFACE -lstdc++
${CPP_LIBRARY} ${CPPABI_LIBRARY})
message(STATUS "Filament C++ libraries: ${CPP_LIBRARY} ${CPPABI_LIBRARY}")
if (LIBCPP_VERSION GREATER 11)
message(WARNING "libc++ (LLVM) version ${LIBCPP_VERSION} > 11 includes libunwind that "
"interferes with the system libunwind.so.8 and may crash Python code when exceptions "
"are used. Please consider using libc++ (LLVM) v11.")
endif()
endif()
if (APPLE)
find_library(CORE_VIDEO CoreVideo)
Expand Down
25 changes: 6 additions & 19 deletions cpp/open3d/core/nns/NanoFlannImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,6 @@ void _KnnSearchCPU(NanoFlannIndexHolderBase *holder,
return;
}

auto points_equal = [](const T *const p1, const T *const p2,
size_t dimension) {
std::vector<T> p1_vec(p1, p1 + dimension);
std::vector<T> p2_vec(p2, p2 + dimension);
return p1_vec == p2_vec;
};

std::vector<std::vector<TIndex>> neighbors_indices(num_queries);
std::vector<std::vector<T>> neighbors_distances(num_queries);
std::vector<uint32_t> neighbors_count(num_queries, 0);
Expand All @@ -147,8 +140,9 @@ void _KnnSearchCPU(NanoFlannIndexHolderBase *holder,
for (size_t valid_i = 0; valid_i < num_valid; ++valid_i) {
TIndex idx = result_indices[valid_i];
if (ignore_query_point &&
points_equal(&queries[i * dimension],
&points[idx * dimension], dimension)) {
std::equal(&queries[i * dimension],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! This should significantly reduce the number of heap allocations

&queries[i * dimension] + dimension,
&points[idx * dimension])) {
continue;
}
neighbors_indices[i].push_back(idx);
Expand Down Expand Up @@ -222,13 +216,6 @@ void _RadiusSearchCPU(NanoFlannIndexHolderBase *holder,
return;
}

auto points_equal = [](const T *const p1, const T *const p2,
size_t dimension) {
std::vector<T> p1_vec(p1, p1 + dimension);
std::vector<T> p2_vec(p2, p2 + dimension);
return p1_vec == p2_vec;
};

std::vector<std::vector<TIndex>> neighbors_indices(num_queries);
std::vector<std::vector<T>> neighbors_distances(num_queries);
std::vector<uint32_t> neighbors_count(num_queries, 0);
Expand All @@ -255,9 +242,9 @@ void _RadiusSearchCPU(NanoFlannIndexHolderBase *holder,
int num_neighbors = 0;
for (const auto &idx_dist : search_result) {
if (ignore_query_point &&
points_equal(&queries[i * dimension],
&points[idx_dist.first * dimension],
dimension)) {
std::equal(&queries[i * dimension],
&queries[i * dimension] + dimension,
&points[idx_dist.first * dimension])) {
continue;
}
neighbors_indices[i].push_back(idx_dist.first);
Expand Down
3 changes: 3 additions & 0 deletions cpp/pybind/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ set(PYTHON_COMPILED_MODULE_DIR
if (APPLE)
set_target_properties(pybind PROPERTIES BUILD_RPATH "@loader_path;@loader_path/..")
elseif (UNIX)
# Use RPATH instead of RUNPATH in pybind so that needed libc++.so can find child dependant libc++abi.so in RPATH
# https://stackoverflow.com/questions/69662319/managing-secondary-dependencies-of-shared-libraries
target_link_options(pybind PRIVATE "LINKER:--disable-new-dtags")
set_target_properties(pybind PROPERTIES BUILD_RPATH "$ORIGIN;$ORIGIN/..")
endif()
set_target_properties(pybind PROPERTIES
Expand Down
7 changes: 7 additions & 0 deletions cpp/tests/t/geometry/TensorMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ TEST_P(TensorMapPermuteDevices, Constructor) {
// Primary key is required.
EXPECT_ANY_THROW(t::geometry::TensorMap());

// Delete primary key.
EXPECT_ANY_THROW(tm0.Erase("positions"));

// Reserved keys.
EXPECT_ANY_THROW(tm0.insert(
{"primary_key", core::Tensor::Zeros({2, 3}, dtype, device)}));

// Iterators.
std::map<std::string, core::Tensor> tensor_map(
{{"positions", core::Tensor::Zeros({10, 3}, dtype, device)},
Expand Down
44 changes: 29 additions & 15 deletions docker/Dockerfile.ci
Original file line number Diff line number Diff line change
Expand Up @@ -67,34 +67,48 @@ RUN if [ "${BUILD_SYCL_MODULE}" = "ON" ]; then \
rm -rf /etc/apt/sources.list.d/oneAPI.list; \
fi

# Dependencies: basic
# Dependencies: basic and python-build
RUN apt-get update && apt-get install -y \
git \
wget \
curl \
build-essential \
pkg-config \
zlib1g \
zlib1g-dev \
libssl-dev \
libbz2-dev \
libreadline-dev \
libsqlite3-dev \
libncursesw5-dev \
xz-utils \
tk-dev \
libxml2-dev \
libxmlsec1-dev \
libffi-dev \
liblzma-dev \
&& rm -rf /var/lib/apt/lists/*

# Miniconda or Intel conda
# The **/open3d/bin paths are used during docker run, in this way docker run
# pyenv or Intel Python
# The pyenv python paths are used during docker run, in this way docker run
# does not need to activate the environment again.
ENV PATH="/root/miniconda3/bin:${PATH}"
ENV PATH="/root/miniconda3/envs/open3d/bin:${PATH}"
# The soft link from the python patch level version to the python mino version
# ensures python wheel commands (i.e. open3d) are in PATH, since we don't know
# which patch level pyenv will install (latest).
ENV PYENV_ROOT=/root/.pyenv
ENV PATH="$PYENV_ROOT/shims:$PYENV_ROOT/bin:$PYENV_ROOT/versions/$PYTHON_VERSION/bin:$PATH"
ENV PATH="/opt/intel/oneapi/intelpython/latest/bin:${PATH}"
ENV PATH="/opt/intel/oneapi/intelpython/latest/envs/open3d/bin:${PATH}"
RUN if [ "${BUILD_SYCL_MODULE}" = "OFF" ]; then \
wget -q https://repo.anaconda.com/miniconda/Miniconda3-latest-Linux-x86_64.sh; \
bash Miniconda3-latest-Linux-x86_64.sh -b; \
rm Miniconda3-latest-Linux-x86_64.sh; \
curl https://pyenv.run | bash \
&& pyenv update \
Copy link
Contributor

Choose a reason for hiding this comment

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

I was working on switching to Miniforge in #6717 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

miniforge likely won't fix this issue, since it's a copy of conda and likely uses it's on libstdc++ as well.

&& pyenv install $PYTHON_VERSION \
&& pyenv global $PYTHON_VERSION \
&& pyenv rehash \
&& ln -s $PYENV_ROOT/versions/${PYTHON_VERSION}* $PYENV_ROOT/versions/${PYTHON_VERSION}; \
fi
RUN conda --version \
&& conda create -y -n open3d python=${PYTHON_VERSION}
RUN python --version && pip --version

# Activate open3d virtualenv
# This works during docker build. It becomes the prefix of all RUN commands.
# Ref: https://stackoverflow.com/a/60148365/1255535
SHELL ["conda", "run", "-n", "open3d", "/bin/bash", "-o", "pipefail", "-c"]
SHELL ["/bin/bash", "-o", "pipefail", "-c"]

# Dependencies: cmake
ENV PATH=${HOME}/${CMAKE_VERSION}/bin:${PATH}
Expand Down
2 changes: 1 addition & 1 deletion docker/docker_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ cpp_python_linking_uninstall_test() {
# Python test
echo "pytest is randomized, add --randomly-seed=SEED to repeat the test sequence."
${docker_run} -i --rm "${DOCKER_TAG}" /bin/bash -c " \
python -m pytest python/test ${pytest_args} -s"
python -W default -m pytest python/test ${pytest_args} -s"
restart_docker_daemon_if_on_gcloud

# Command-line tools test
Expand Down
8 changes: 0 additions & 8 deletions python/open3d/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,6 @@ def load_cdll(path):
if sys.platform == "win32": # Unix: Use rpath to find libraries
_win32_dll_dir = os.add_dll_directory(str(Path(__file__).parent))

if _build_config["BUILD_GUI"] and not (find_library("c++abi") or
find_library("c++")):
try: # Preload libc++.so and libc++abi.so (required by filament)
load_cdll(str(next((Path(__file__).parent).glob("*c++abi.*"))))
load_cdll(str(next((Path(__file__).parent).glob("*c++.*"))))
except StopIteration: # Not found: check system paths while loading
pass

__DEVICE_API__ = "cpu"
if _build_config["BUILD_CUDA_MODULE"]:
# Load CPU pybind dll gracefully without introducing new python variable.
Expand Down
11 changes: 8 additions & 3 deletions util/install_deps_ubuntu.sh
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,22 @@ eval $(
echo DISTRIB_ID="$DISTRIB_ID";
echo DISTRIB_RELEASE="$DISTRIB_RELEASE"
)
# To avoid dependence on libunwind, we don't want to use clang / libc++ versions later than 11.
# Ubuntu 20.04's has versions 8, 10 or 12 while Ubuntu 22.04 has versions 11 and later.
if [ "$DISTRIB_ID" == "Ubuntu" -a "$DISTRIB_RELEASE" == "20.04" ]; then
# Ubuntu 20.04's clang/libc++-dev/libc++abi-dev are version 8, 10 or 12.
# To avoid dependence on libunwind, we don't want to use versions later than 10.
deps=("${deps[@]/clang/clang-10}")
deps=("${deps[@]/libc++-dev/libc++-10-dev}")
deps=("${deps[@]/libc++abi-dev/libc++abi-10-dev}")
fi
if [ "$DISTRIB_ID" == "Ubuntu" -a "$DISTRIB_RELEASE" == "22.04" ]; then
deps=("${deps[@]/clang/clang-11}")
deps=("${deps[@]/libc++-dev/libc++-11-dev}")
deps=("${deps[@]/libc++abi-dev/libc++abi-11-dev}")
fi

# Special case for ARM64
if [ "$(uname -m)" == "aarch64" ]; then
# For compling LAPACK in OpenBLAS
# For compiling LAPACK in OpenBLAS
deps+=("gfortran")
fi

Expand Down
Loading