Merge pull request #1685 from talregev/TalR/test_python_windows

Author: varunagrawal

.github/scripts/python.sh

@@ -65,14 +65,21 @@ 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
+
+  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

.github/workflows/build-python.yml

@@ -18,6 +18,8 @@ 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
@@ -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,68 @@ 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
+        shell: bash
         run: |
           bash .github/scripts/python.sh -t

gtsam/base/utilities.h

@@ -4,14 +4,16 @@
 #include <iostream>
 #include <sstream>
 
+#include <gtsam/dllexport.h>
+
 namespace gtsam {
 /**
  * For Python __str__().
  * Redirect std cout to a string stream so we can return a string representation
  * 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())) {}
 

gtsam/discrete/DiscreteValues.h

@@ -126,12 +126,12 @@ inline std::vector<DiscreteValues> 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 = {});
 

gtsam/geometry/Rot3.h

@@ -396,7 +396,7 @@ class GTSAM_EXPORT Rot3 : public LieGroup<Rot3, 3> {
     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 = {});
     };

gtsam/nonlinear/CustomFactor.h

@@ -42,7 +42,7 @@ using CustomErrorFunction = std::function<Vector(const CustomFactor &, const Val
  *
  * This factor is mainly for creating a custom factor in Python.
  */
-class CustomFactor: public NoiseModelFactor {
+class GTSAM_EXPORT CustomFactor: public NoiseModelFactor {
 protected:
   CustomErrorFunction error_function_;
 

gtsam/nonlinear/Values-inl.h

@@ -197,6 +197,61 @@ 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 <int M, int N>
+   struct handle_matrix<Eigen::Matrix<double, M, N, 0, M, N>, true> {
+     inline Eigen::Matrix<double, M, N> operator()(Key j, const Value* const pointer) {
+       auto ptr = dynamic_cast<const GenericValue<Eigen::Matrix<double, M, N>>*>(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<double, M, N>));
+       }
+     }
+   };
+
+   // Handle fixed matrices
+   template <int M, int N>
+   struct handle_matrix<Eigen::Matrix<double, M, N, 0, M, N>, false> {
+     inline Eigen::Matrix<double, M, N> operator()(Key j, const Value* const pointer) {
+       auto ptr = dynamic_cast<const GenericValue<Eigen::Matrix<double, M, N>>*>(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<const GenericValue<Eigen::MatrixXd>*>(pointer);
+         if (ptr) {
+           A = ptr->value();
+         } else {
+           // Or a dynamic vector
+           A = handle_matrix<Eigen::VectorXd, true>()(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 <int M, int N>
+   struct handle<Eigen::Matrix<double, M, N, 0, M, N>> {
+     Eigen::Matrix<double, M, N> operator()(Key j, const Value* const pointer) {
+       return handle_matrix<Eigen::Matrix<double, M, N, 0, M, N>,
+                            (M == Eigen::Dynamic || N == Eigen::Dynamic)>()(j, pointer);
+     }
+   };
+#endif // #ifdef _WIN32
+
    }  // internal
 
    /* ************************************************************************* */

gtsam/sfm/DsfTrackGenerator.h

@@ -69,7 +69,7 @@ using MatchIndicesMap = std::map<IndexPair, CorrespondenceIndices>;
  *        correspondence indices, from each image.
  * @param Length-N list of keypoints, for N images/cameras.
  */
-std::vector<SfmTrack2d> tracksFromPairwiseMatches(
+std::vector<SfmTrack2d> GTSAM_EXPORT tracksFromPairwiseMatches(
     const MatchIndicesMap& matches, const KeypointsVector& keypoints,
     bool verbose = false);
 

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
+        "$<TARGET_FILE:gtsam>;$<TARGET_RUNTIME_DLLS:gtsam>"
+        "${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
+            "$<TARGET_FILE:gtsam_unstable>;$<TARGET_RUNTIME_DLLS:gtsam_unstable>"
+            "${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})
 

python/setup.py.in

@@ -11,7 +11,8 @@ print("PACKAGES: ", packages)
 package_data = {
     '': [
         "./*.so",
-        "./*.dll"
+        "./*.dll",
+        "./*.pyd",
     ]
 }