From 2971d9e74ebac099e7741b78612fd5a69ece4035 Mon Sep 17 00:00:00 2001 From: Tal Regev Date: Fri, 8 Dec 2023 01:25:41 +0200 Subject: [PATCH 1/5] Compile gtsam python for windows --- .github/scripts/python.sh | 7 ++-- .github/workflows/build-python.yml | 55 +++++++++++++++++++++++++-- gtsam/base/utilities.h | 4 +- gtsam/discrete/DiscreteValues.h | 4 +- gtsam/geometry/Rot3.h | 2 +- gtsam/nonlinear/CustomFactor.h | 2 +- gtsam/sfm/DsfTrackGenerator.h | 2 +- python/CMakeLists.txt | 60 ++++++++++++++++++++++++++++-- python/setup.py.in | 3 +- 9 files changed, 121 insertions(+), 18 deletions(-) diff --git a/.github/scripts/python.sh b/.github/scripts/python.sh index 08b8084a05..c851c7e36c 100644 --- a/.github/scripts/python.sh +++ b/.github/scripts/python.sh @@ -65,14 +65,13 @@ function build() # Set to 2 cores so that Actions does not error out during resource provisioning. cmake --build build -j2 - $PYTHON -m pip install --user build/python + cmake --build build --target python-install } function test() { - cd $GITHUB_WORKSPACE/python/gtsam/tests - $PYTHON -m unittest discover -v - cd $GITHUB_WORKSPACE + cmake --build build --target python-test + cmake --build build --target python-test-unstable } # select between build or test diff --git a/.github/workflows/build-python.yml b/.github/workflows/build-python.yml index 91bc4e80ac..676fe8ea09 100644 --- a/.github/workflows/build-python.yml +++ b/.github/workflows/build-python.yml @@ -18,9 +18,11 @@ jobs: CTEST_PARALLEL_LEVEL: 2 CMAKE_BUILD_TYPE: ${{ matrix.build_type }} PYTHON_VERSION: ${{ matrix.python_version }} + BOOST_VERSION: 1.72.0 + BOOST_EXE: boost_1_72_0-msvc-14.2 strategy: - fail-fast: true + fail-fast: false matrix: # Github Actions requires a single row to be added to the build matrix. # See https://help.github.com/en/articles/workflow-syntax-for-github-actions. @@ -30,6 +32,7 @@ jobs: ubuntu-20.04-gcc-9-tbb, ubuntu-20.04-clang-9, macOS-11-xcode-13.4.1, + windows-2019-msbuild, ] build_type: [Release] @@ -56,6 +59,10 @@ jobs: compiler: xcode version: "13.4.1" + - name: windows-2019-msbuild + os: windows-2019 + platform: 64 + steps: - name: Checkout uses: actions/checkout@v3 @@ -97,29 +104,71 @@ jobs: echo "CC=clang" >> $GITHUB_ENV echo "CXX=clang++" >> $GITHUB_ENV + - name: Setup msbuild (Windows) + if: runner.os == 'Windows' + uses: ilammy/msvc-dev-cmd@v1 + with: + arch: x${{matrix.platform}} + + - name: Setup python (Windows) + uses: actions/setup-python@v4 + if: runner.os == 'Windows' + with: + python-version: ${{ matrix.python_version }} + + - name: Install ninja (Windows) + if: runner.os == 'Windows' + shell: bash + run: | + choco install ninja + ninja --version + where ninja + + - name: Install Boost (Windows) + if: runner.os == 'Windows' + shell: powershell + run: | + # Snippet from: https://github.com/actions/virtual-environments/issues/2667 + $BOOST_PATH = "C:\hostedtoolcache\windows\Boost\$env:BOOST_VERSION\x86_64" + + # Use the prebuilt binary for Windows + $Url = "https://sourceforge.net/projects/boost/files/boost-binaries/$env:BOOST_VERSION/$env:BOOST_EXE-${{matrix.platform}}.exe" + (New-Object System.Net.WebClient).DownloadFile($Url, "$env:TEMP\boost.exe") + Start-Process -Wait -FilePath "$env:TEMP\boost.exe" "/SILENT","/SP-","/SUPPRESSMSGBOXES","/DIR=$BOOST_PATH" + + # Set the BOOST_ROOT variable + echo "BOOST_ROOT=$BOOST_PATH" >> $env:GITHUB_ENV + - name: Set GTSAM_WITH_TBB Flag if: matrix.flag == 'tbb' run: | echo "GTSAM_WITH_TBB=ON" >> $GITHUB_ENV echo "GTSAM Uses TBB" - - name: Set Swap Space + - name: Set Swap Space (Linux) if: runner.os == 'Linux' uses: pierotofy/set-swap-space@master with: swap-size-gb: 6 - - name: Install System Dependencies + - name: Install System Dependencies (Linux, macOS) + if: runner.os != 'Windows' run: | bash .github/scripts/python.sh -d - name: Install Python Dependencies + shell: bash run: python$PYTHON_VERSION -m pip install -r python/dev_requirements.txt - name: Build + shell: bash run: | bash .github/scripts/python.sh -b - name: Test + # Disable running tests for windows because some of them are failing. + # Remove this condition when you want to run tests on windows CI. + if: runner.os != 'Windows' + shell: bash run: | bash .github/scripts/python.sh -t diff --git a/gtsam/base/utilities.h b/gtsam/base/utilities.h index 03e9636da4..a67c5d1b6c 100644 --- a/gtsam/base/utilities.h +++ b/gtsam/base/utilities.h @@ -4,6 +4,8 @@ #include #include +#include + namespace gtsam { /** * For Python __str__(). @@ -11,7 +13,7 @@ namespace gtsam { * of an object when it prints to cout. * https://stackoverflow.com/questions/5419356/redirect-stdout-stderr-to-a-string */ -struct RedirectCout { +struct GTSAM_EXPORT RedirectCout { /// constructor -- redirect stdout buffer to a stringstream buffer RedirectCout() : ssBuffer_(), coutBuffer_(std::cout.rdbuf(ssBuffer_.rdbuf())) {} diff --git a/gtsam/discrete/DiscreteValues.h b/gtsam/discrete/DiscreteValues.h index 9ec08302bd..9fdff014cc 100644 --- a/gtsam/discrete/DiscreteValues.h +++ b/gtsam/discrete/DiscreteValues.h @@ -126,12 +126,12 @@ inline std::vector cartesianProduct(const DiscreteKeys& keys) { } /// Free version of markdown. -std::string markdown(const DiscreteValues& values, +std::string GTSAM_EXPORT markdown(const DiscreteValues& values, const KeyFormatter& keyFormatter = DefaultKeyFormatter, const DiscreteValues::Names& names = {}); /// Free version of html. -std::string html(const DiscreteValues& values, +std::string GTSAM_EXPORT html(const DiscreteValues& values, const KeyFormatter& keyFormatter = DefaultKeyFormatter, const DiscreteValues::Names& names = {}); diff --git a/gtsam/geometry/Rot3.h b/gtsam/geometry/Rot3.h index 2b9c5a45a8..7e05ee4daf 100644 --- a/gtsam/geometry/Rot3.h +++ b/gtsam/geometry/Rot3.h @@ -396,7 +396,7 @@ class GTSAM_EXPORT Rot3 : public LieGroup { Matrix3 AdjointMap() const { return matrix(); } // Chart at origin, depends on compile-time flag ROT3_DEFAULT_COORDINATES_MODE - struct ChartAtOrigin { + struct GTSAM_EXPORT ChartAtOrigin { static Rot3 Retract(const Vector3& v, ChartJacobian H = {}); static Vector3 Local(const Rot3& r, ChartJacobian H = {}); }; diff --git a/gtsam/nonlinear/CustomFactor.h b/gtsam/nonlinear/CustomFactor.h index ac29420320..c4015db373 100644 --- a/gtsam/nonlinear/CustomFactor.h +++ b/gtsam/nonlinear/CustomFactor.h @@ -42,7 +42,7 @@ using CustomErrorFunction = std::function; * correspondence indices, from each image. * @param Length-N list of keypoints, for N images/cameras. */ -std::vector tracksFromPairwiseMatches( +std::vector GTSAM_EXPORT tracksFromPairwiseMatches( const MatchIndicesMap& matches, const KeypointsVector& keypoints, bool verbose = false); diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index f874c2f217..ba55ac2af2 100644 --- a/python/CMakeLists.txt +++ b/python/CMakeLists.txt @@ -94,6 +94,14 @@ set(interface_headers set(GTSAM_PYTHON_TARGET gtsam_py) set(GTSAM_PYTHON_UNSTABLE_TARGET gtsam_unstable_py) +set(GTSAM_OUTPUT_NAME "gtsam") +set(GTSAM_UNSTABLE_OUTPUT_NAME "gtsam_unstable") + +if(MSVC) + set(GTSAM_OUTPUT_NAME "gtsam_py") + set(GTSAM_UNSTABLE_OUTPUT_NAME "gtsam_unstable_py") +endif() + pybind_wrap(${GTSAM_PYTHON_TARGET} # target "${interface_headers}" # interface_headers "gtsam.cpp" # generated_cpp @@ -109,12 +117,30 @@ pybind_wrap(${GTSAM_PYTHON_TARGET} # target set_target_properties(${GTSAM_PYTHON_TARGET} PROPERTIES INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib" INSTALL_RPATH_USE_LINK_PATH TRUE - OUTPUT_NAME "gtsam" + OUTPUT_NAME "${GTSAM_OUTPUT_NAME}" LIBRARY_OUTPUT_DIRECTORY "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam" DEBUG_POSTFIX "" # Otherwise you will have a wrong name RELWITHDEBINFO_POSTFIX "" # Otherwise you will have a wrong name ) +if(WIN32) + set_target_properties(${GTSAM_PYTHON_TARGET} PROPERTIES + SUFFIX ".pyd" + ) + ADD_CUSTOM_COMMAND(TARGET ${GTSAM_PYTHON_TARGET} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different + "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam/${GTSAM_OUTPUT_NAME}.pyd" + "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam/gtsam.pyd" + ) + ADD_CUSTOM_COMMAND(TARGET ${GTSAM_PYTHON_TARGET} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different + "$;$" + "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam/" + COMMAND_EXPAND_LISTS + VERBATIM + ) +endif() + # Set the path for the GTSAM python module set(GTSAM_MODULE_PATH ${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam) @@ -188,7 +214,7 @@ if(GTSAM_UNSTABLE_BUILD_PYTHON) set_target_properties(${GTSAM_PYTHON_UNSTABLE_TARGET} PROPERTIES INSTALL_RPATH "${CMAKE_INSTALL_PREFIX}/lib" INSTALL_RPATH_USE_LINK_PATH TRUE - OUTPUT_NAME "gtsam_unstable" + OUTPUT_NAME "${GTSAM_UNSTABLE_OUTPUT_NAME}" LIBRARY_OUTPUT_DIRECTORY "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam_unstable" DEBUG_POSTFIX "" # Otherwise you will have a wrong name RELWITHDEBINFO_POSTFIX "" # Otherwise you will have a wrong name @@ -208,13 +234,39 @@ if(GTSAM_UNSTABLE_BUILD_PYTHON) # Add gtsam_unstable to the install target list(APPEND GTSAM_PYTHON_DEPENDENCIES ${GTSAM_PYTHON_UNSTABLE_TARGET}) - + if(WIN32) + set_target_properties(${GTSAM_PYTHON_UNSTABLE_TARGET} PROPERTIES + SUFFIX ".pyd" + ) + ADD_CUSTOM_COMMAND(TARGET ${GTSAM_PYTHON_UNSTABLE_TARGET} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different + "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam_unstable/${GTSAM_UNSTABLE_OUTPUT_NAME}.pyd" + "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam_unstable/gtsam_unstable.pyd" + ) + ADD_CUSTOM_COMMAND(TARGET ${GTSAM_PYTHON_UNSTABLE_TARGET} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different + "$;$" + "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam_unstable/" + COMMAND_EXPAND_LISTS + VERBATIM + ) + endif() + # Custom make command to run all GTSAM_UNSTABLE Python tests + add_custom_target( + python-test-unstable + COMMAND + ${CMAKE_COMMAND} -E env # add package to python path so no need to install + "PYTHONPATH=${GTSAM_PYTHON_BUILD_DIRECTORY}/$ENV{PYTHONPATH}" + ${PYTHON_EXECUTABLE} -m unittest discover -v -s . + DEPENDS ${GTSAM_PYTHON_DEPENDENCIES} ${GTSAM_PYTHON_TEST_FILES} + WORKING_DIRECTORY "${GTSAM_PYTHON_BUILD_DIRECTORY}/gtsam_unstable/tests" + ) endif() # Add custom target so we can install with `make python-install` set(GTSAM_PYTHON_INSTALL_TARGET python-install) add_custom_target(${GTSAM_PYTHON_INSTALL_TARGET} - COMMAND ${PYTHON_EXECUTABLE} -m pip install . + COMMAND ${PYTHON_EXECUTABLE} -m pip install --user . DEPENDS ${GTSAM_PYTHON_DEPENDENCIES} WORKING_DIRECTORY ${GTSAM_PYTHON_BUILD_DIRECTORY}) diff --git a/python/setup.py.in b/python/setup.py.in index e15e390754..824a6656ed 100644 --- a/python/setup.py.in +++ b/python/setup.py.in @@ -11,7 +11,8 @@ print("PACKAGES: ", packages) package_data = { '': [ "./*.so", - "./*.dll" + "./*.dll", + "./*.pyd", ] } From 8023df456d9a4c59ce23979a2ab32953dfc520e3 Mon Sep 17 00:00:00 2001 From: Tal Regev Date: Sun, 31 Dec 2023 23:19:29 +0200 Subject: [PATCH 2/5] add windows template specialization --- .github/workflows/build-python.yml | 3 -- gtsam/nonlinear/Values-inl.h | 53 ++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-python.yml b/.github/workflows/build-python.yml index 676fe8ea09..bb10b27c38 100644 --- a/.github/workflows/build-python.yml +++ b/.github/workflows/build-python.yml @@ -166,9 +166,6 @@ jobs: bash .github/scripts/python.sh -b - name: Test - # Disable running tests for windows because some of them are failing. - # Remove this condition when you want to run tests on windows CI. - if: runner.os != 'Windows' shell: bash run: | bash .github/scripts/python.sh -t diff --git a/gtsam/nonlinear/Values-inl.h b/gtsam/nonlinear/Values-inl.h index a93f9570e6..6da87d71c2 100644 --- a/gtsam/nonlinear/Values-inl.h +++ b/gtsam/nonlinear/Values-inl.h @@ -197,6 +197,59 @@ namespace gtsam { } }; +#ifdef _WIN32 + // Handle dynamic matrices + template + struct handle_matrix, true> { + inline Eigen::Matrix operator()(Key j, const Value* const pointer) { + auto ptr = dynamic_cast>*>(pointer); + if (ptr) { + // value returns a const Matrix&, and the return makes a copy !!!!! + return ptr->value(); + } else { + // If a fixed matrix was stored, we end up here as well. + throw ValuesIncorrectType(j, typeid(*pointer), typeid(Eigen::Matrix)); + } + } + }; + + // Handle fixed matrices + template + struct handle_matrix, false> { + inline Eigen::Matrix operator()(Key j, const Value* const pointer) { + auto ptr = dynamic_cast>*>(pointer); + if (ptr) { + // value returns a const MatrixMN&, and the return makes a copy !!!!! + return ptr->value(); + } else { + Matrix A; + // Check if a dynamic matrix was stored + auto ptr = dynamic_cast*>(pointer); + if (ptr) { + A = ptr->value(); + } else { + // Or a dynamic vector + A = handle_matrix()(j, pointer); // will throw if not.... + } + // Yes: check size, and throw if not a match + if (A.rows() != M || A.cols() != N) + throw NoMatchFoundForFixed(M, N, A.rows(), A.cols()); + else + return A; // copy but not malloc + } + } + }; + + // Handle matrices + template + struct handle> { + Eigen::Matrix operator()(Key j, const Value* const pointer) { + return handle_matrix, + (M == Eigen::Dynamic || N == Eigen::Dynamic)>()(j, pointer); + } + }; +#endif // #ifdef _WIN32 + } // internal /* ************************************************************************* */ From 85cae70cefe58d5ffb336e8fb77f73fc83c8b753 Mon Sep 17 00:00:00 2001 From: Tal Regev Date: Thu, 4 Jan 2024 08:51:19 +0200 Subject: [PATCH 3/5] revert lines according to review comments. --- .github/scripts/python.sh | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/.github/scripts/python.sh b/.github/scripts/python.sh index c851c7e36c..0c04eeec3d 100644 --- a/.github/scripts/python.sh +++ b/.github/scripts/python.sh @@ -70,8 +70,16 @@ function build() function test() { - cmake --build build --target python-test - cmake --build build --target python-test-unstable + cd $GITHUB_WORKSPACE/python/gtsam/tests + $PYTHON -m unittest discover -v + cd $GITHUB_WORKSPACE + + cd $GITHUB_WORKSPACE/python/gtsam_unstable/tests + $PYTHON -m unittest discover -v + cd $GITHUB_WORKSPACE + + # cmake --build build --target python-test + # cmake --build build --target python-test-unstable } # select between build or test From d1ab94f51c979652b924b2323dd3e3e4a2f58655 Mon Sep 17 00:00:00 2001 From: Tal Regev Date: Fri, 5 Jan 2024 11:05:03 +0200 Subject: [PATCH 4/5] Add comments explain the windows workaround. --- gtsam/nonlinear/Values-inl.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gtsam/nonlinear/Values-inl.h b/gtsam/nonlinear/Values-inl.h index 6da87d71c2..1fe909a110 100644 --- a/gtsam/nonlinear/Values-inl.h +++ b/gtsam/nonlinear/Values-inl.h @@ -197,6 +197,8 @@ namespace gtsam { } }; +// Added this section for compile gtsam python on windows. +// msvc don't deduct the template arguments correctly, due possible bug in msvc. #ifdef _WIN32 // Handle dynamic matrices template From b104fd66901d63ce27949a311bf2b720d78e9d75 Mon Sep 17 00:00:00 2001 From: Tal Regev Date: Tue, 16 Jan 2024 22:36:08 +0200 Subject: [PATCH 5/5] fail-fast: true --- .github/workflows/build-python.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build-python.yml b/.github/workflows/build-python.yml index bb10b27c38..520e94c09b 100644 --- a/.github/workflows/build-python.yml +++ b/.github/workflows/build-python.yml @@ -22,7 +22,7 @@ jobs: BOOST_EXE: boost_1_72_0-msvc-14.2 strategy: - fail-fast: false + fail-fast: true matrix: # Github Actions requires a single row to be added to the build matrix. # See https://help.github.com/en/articles/workflow-syntax-for-github-actions.