Merge pull request #422 from astro-informatics/debugging

Various build fixes from LEXCI in-person meeting
astro-informatics · May 28, 2024 · 2d53780 · 2d53780
2 parents a293d1e + a32a0a2
commit 2d53780
Show file tree

Hide file tree

Showing 32 changed files with 498 additions and 302 deletions.
diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml
@@ -38,7 +38,7 @@ jobs:
       matrix:
           os:
             - ubuntu-20.04
-            - macos-12
+            - macos-14
           cc:
             - gcc-10
             - clang
@@ -59,10 +59,10 @@ jobs:
             - os: ubuntu-20.04
               cc: clang
               cxx: clang++
-            - os: macos-12
+            - os: macos-14
               mpi: "on"
             # This doesn't seem to work with ONNXrt yet:
-            - os: macos-12
+            - os: macos-14
               cxx: g++-10
 
     steps:
@@ -75,12 +75,15 @@ jobs:
       run: |
         sudo apt update
         sudo apt install openmpi-bin libopenmpi-dev libyaml-cpp-dev ccache
+        pip install conan
 
     - name: Install Dependencies on MacOS
       if: ${{ contains(matrix.os, 'macos') }}
       run: |
-        brew install libtiff open-mpi libyaml ccache conan
-        echo "CMAKE_PREFIX_PATH=/usr/local/opt/libomp" >> $GITHUB_ENV
+        brew install libtiff libomp open-mpi libyaml ccache conan
+        echo "CMAKE_PREFIX_PATH=/opt/homebrew/opt/libomp" >> $GITHUB_ENV
+        echo "/opt/homebrew/opt/ccache/libexec" >> $GITHUB_PATH
+        #echo "/opt/homebrew/opt/libomp" >> $GITHUB_PATH
 
     #- name: Install gcc on MacOS
     #  if: ${{ contains(matrix.os, 'macos') && contains(matrix.cxx, 'g++10') }}
@@ -108,20 +111,12 @@ jobs:
     #    os: darwin
 
     - name: Select Python 3.10
-      # otherwise turtlebrowser/[email protected] fails on macos-12
+      # otherwise turtlebrowser/[email protected] fails on macos-14
       # ref: https://github.com/turtlebrowser/get-conan/issues/4
       uses: actions/setup-python@v4
       with:
         python-version: '3.10'
 
-    - name: Install Conan
-      id: conan
-      if: ${{ contains(matrix.os, 'ubuntu') }}
-      uses: turtlebrowser/get-conan@main
-      with:
-        #version: 1.59.0
-        version: 2.0.9
-
     - name: Prepare ccache timestamp
       id: ccache_cache_timestamp
       run: echo "{date_and_time}={$(date +'%Y-%m-%d-%H;%M;%S')}" >> $GITHUB_OUTPUT
@@ -161,22 +156,15 @@ jobs:
     #    sed -i'' -e "s|generators|#generators|1" ./cppflow/conanfile.py
     #    conan create ./cppflow/ -s compiler.cppstd=gnu17
 
-    - name: Conan install on gcc
-      if: ${{ contains(matrix.cxx, 'g++-10') }}
-      run: |
-        conan profile detect
-        conan install ${{github.workspace}} -of ${{github.workspace}}/build --build missing -o mpi=${{matrix.mpi}} -o openmp=${{matrix.omp}} -o cppflow=off -o onnxrt=on -s compiler.cppstd=17
-
-    - name: Conan install on apple-clang
-      if: ${{ contains(matrix.cxx, 'clang++') }}
+    - name: Dependencies
       run: |
         conan profile detect
-        conan install ${{github.workspace}} -of ${{github.workspace}}/build --build missing -o mpi=${{matrix.mpi}} -o openmp=${{matrix.omp}} -o cppflow=off -o onnxrt=on -s compiler.cppstd=17
+        conan install ${{github.workspace}} -of ${{github.workspace}}/build --build missing -o dompi=${{matrix.mpi}} -o openmp=${{matrix.omp}} -o cppflow=off -o onnxrt=on -s compiler.cppstd=17
 
     - name: Build
       # Build your program with the given configuration.
       # The Github Actions machines are dual-core so we can build faster using 2 parallel processes
-      run: conan build ${{github.workspace}} -of ${{github.workspace}}/build -o mpi=${{matrix.mpi}} -o openmp=${{matrix.omp}} -o cppflow=off -o onnxrt=on -s compiler.cppstd=17
+      run: conan build ${{github.workspace}} -of ${{github.workspace}}/build -o dompi=${{matrix.mpi}} -o openmp=${{matrix.omp}} -o cppflow=off -o onnxrt=on -s compiler.cppstd=17
 
     - name: Test
       working-directory: ${{github.workspace}}/build

diff --git a/.github/workflows/documentation.yml b/.github/workflows/documentation.yml
@@ -38,14 +38,15 @@ jobs:
       # This is because there is no easy way of extracting the path to the doxygen executable from conan.
       # the apt installed doxygen is used in the Make Docweb step to build the html files,
       # while the conan installed version is used in the build step.
-      # The version we install from apt is older than the 1.9.4 we install from conan, but
-      # it is not causing any issues at the moment.
+      # -> Is this still true for Conan v2?
       - name: Install dependencies
         run: |
           sudo apt update
           sudo apt install openmpi-bin libopenmpi-dev libyaml-cpp-dev doxygen graphviz ccache
-          if [[ "$USE_CONAN" = 0 ]]; then
-            sudo apt install libeigen3-dev libspdlog-dev libtiff-dev
+          if [[ "$USE_CONAN" = 1 ]]; then
+            pip install conan
+          else
+            sudo apt install libeigen3-dev libtiff-dev
             git clone https://github.com/catchorg/Catch2.git -b v3.4.0
             mkdir Catch2/build
             cd Catch2/build
@@ -55,14 +56,6 @@ jobs:
             cd -
           fi
 
-      - name: Install Conan
-        id: conan
-        if: env.USE_CONAN != '0'
-        uses: turtlebrowser/get-conan@main
-        with:
-          version: 1.60.2
-          #version: 2.0.9
-
       - name: Install Tensorflow API on Ubuntu
         uses: UCL/install-tensorflow-action@main
         with:
@@ -79,9 +72,9 @@ jobs:
       - name: Create cppflow package
         run: |
           if [[ "$USE_CONAN" = 1 ]]; then
-            #conan profile detect
-            #sed -i'' -e "s|generators|#generators|1" ./cppflow/conanfile.py
-            conan create ./cppflow/ -pr:h=default -pr:b=default
+            conan profile detect
+            sed -i'' -e "s|generators|#generators|1" ./cppflow/conanfile.py
+            conan create ./cppflow/
           else
             mkdir cppflow/build
             cd cppflow/build
@@ -93,9 +86,7 @@ jobs:
       - name: Configure
         run: |
           if [[ "$USE_CONAN" = 1 ]]; then
-            # Doxygen currently broken in Conan v1 and v2
-            #conan install doxygen/1.9.4@#2af713e135f12722e3536808017ba086 --update
-            conan install ${{github.workspace}} -if ${{github.workspace}}/build --build missing -s compiler.libcxx=libstdc++11 -o mpi=off -o openmp=off -o docs=off -o cppflow=on -pr:h=default -pr:b=default
+            conan install ${{github.workspace}} -of ${{github.workspace}}/build --build missing -s compiler.libcxx=libstdc++11 -o dompi=off -o openmp=off -o docs=off -o cppflow=on
           else
             export PATH=${{github.workspace}}/cppflow/build:$PATH
             export PATH=${{github.workspace}}/Catch2/build:$PATH
@@ -108,7 +99,7 @@ jobs:
       - name: Build
         run: |
           if [[ "$USE_CONAN" = 1 ]]; then
-            conan build ${{github.workspace}} -bf ${{github.workspace}}/build
+            conan build ${{github.workspace}} -of ${{github.workspace}}/build
           else
             export PATH=${{github.workspace}}/cppflow/build:$PATH
             export PATH=${{github.workspace}}/Catch2/build:$PATH

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -59,7 +59,6 @@ set(SOPT_GITREF ${GIT_HASH})
 option(tests          "Enable testing"                         on)
 option(benchmarks     "Enable benchmarking"                    off)
 option(examples       "Enable Examples"                        on)
-option(logging        "Enable logging"                         on)
 option(openmp         "Enable OpenMP"                          on)
 option(dompi          "Enable MPI"                             on)
 option(docs           "Build documentation"                    off)

diff --git a/README.md b/README.md
@@ -27,8 +27,6 @@ This documentation outlines the necessary and optional [dependencies](#dependenc
 - [OpenMP](http://openmp.org/wp/) v4.8.4 (Trusty) - Optional - Speeds up some of the operations.
 - [Cppflow](https://github.com/UCL/cppflow) v2.0.0 - Optional - A warpper for the Tensorflow C API allowing us to read Tensorflow models into SOPT. Needed if you are using a learned prior.
 - [Eigen3](http://eigen.tuxfamily.org/index.php?title=Main_Page) v3.4.0 (Trusty) Modern `C++` linear algebra. Downloaded automatically if absent.
-- [spdlog](https://github.com/gabime/spdlog) v1.12.0 - Optional - Logging library. Downloaded automatically if
-    absent.
 - [Catch2](https://github.com/catchorg/Catch2) v3.4.0 - Optional -  A `C++`
     unit-testing framework only needed for testing. Downloaded automatically if absent.
 - [google/benchmark](https://github.com/google/benchmark) - Optional - A `C++`
@@ -51,7 +49,7 @@ as well as the **SOPT** installation:
         conan create ./cppflow/
         ```
       Note that conan requires you to specify the host (h) and the build (b) profiles on the command
-      line (`-pr:h=default -pr:b=default`), unless you have defined them in your conan profile.
+      line (`-pr:b=default -pr:h=default` or simply `-pr:a=default`), unless you have defined them in your conan profile.
       You can set up a default profile for your system using `conan profile detect` (only needs to be done once).
 
 1. Once the mandatory dependencies are present, `git clone` from the [GitHub repository](https://github.com/astro-informatics/sopt):
@@ -84,17 +82,16 @@ Possible options are:
     - tests (default on)
     - benchmarks (default off)
     - examples (default on)
-    - logging (default on)
-    - openmp (default on)
-    - mpi (default on)
+    - openmp (default off)
+    - dompi (default off)
     - docs (default off)
     - coverage (default off)
     - cppflow (default off)
 
-For example, to build with both MPI and OpenMP off you would use
+For example, to build with both MPI and OpenMP on you would use
 
 ``` bash
-conan install .. -of . --build missing -o openmp=off -o mpi=off -pr:h=default -pr:b=default
+conan install .. -of . --build missing -o openmp=on -o dompi=on
 conan build .. -of .
 ```
 
@@ -115,16 +112,11 @@ If the dependencies are already available on your system, you can also install *
 On MacOS, you can also install most of the dependencies with Homebrew e.g.
 
   ``` bash
-  brew install libtensorflow eigen tiff spdlog catch2
+  brew install libtensorflow eigen tiff catch2
   ```
 **Note that the ONNXruntime interface is currently only supported when compiling with Clang on MacOS, but not with g++**
 
 
-## Common errors
-
-If you are using the g++ compiler and get an error to do with the package `spdlog`, try adding the option `-s compiler.libcxx=libstdc++11` to the `conan build` command. This option is also necessary when building with gcc on MacOS.
-
-
 ## Conan tips
 
 You can set commonly used options, choices of compilers etc. in a
@@ -193,7 +185,7 @@ If you use **SOPT** for work that results in publication, please reference the [
 ## License
 
 > SOPT: Sparse OPTimisation package
-> Copyright (C) 2013-2023
+> Copyright (C) 2013-2024
 >
 > This program is free software; you can redistribute it and/or
 > modify it under the terms of the GNU General Public License as

diff --git a/cmake_files/dependencies.cmake b/cmake_files/dependencies.cmake
@@ -16,13 +16,6 @@ if(NOT TIFF_FOUND)
   message(FATAL_ERROR "TIFF is required")
 endif()
 
-if(logging)
-  find_package(spdlog)
-  if(NOT spdlog_FOUND)
-    message(FATAL_ERROR "logging requires spdlog")
-  endif()
-endif()
-
 if(docs)
   find_package(Doxygen REQUIRED dot)
   if(NOT Doxygen_FOUND)
@@ -35,10 +28,6 @@ if(openmp)
   if(OPENMP_FOUND)
     set(SOPT_DEFAULT_OPENMP_THREADS 2 CACHE STRING "Number of threads used in testing")
     set(SOPT_OPENMP TRUE)
-    add_library(openmp::openmp INTERFACE IMPORTED GLOBAL)
-    set_target_properties(openmp::openmp PROPERTIES
-      INTERFACE_COMPILE_OPTIONS "${OpenMP_CXX_FLAGS}"
-      INTERFACE_LINK_LIBRARIES  "${OpenMP_CXX_FLAGS}")
   else()
     message(FATAL_ERROR "OpenMP was requested but could not be found")
   endif()

diff --git a/conanfile.py b/conanfile.py
@@ -19,28 +19,23 @@ class soptConan(ConanFile):
                "examples":['on','off'],
                "tests":['on','off'],
                "benchmarks":['on','off'],
-               "logging":['on','off'],
                "openmp":['on','off'],
-               "mpi":['on','off'],
+               "dompi":['on','off'],
                "coverage":['on','off'],
                "onnxrt":['on','off'],
                "cppflow":['on','off'],}
     default_options = {"docs": 'off',
                        "examples":'on',
                        "tests": 'on',
                        "benchmarks": 'off',
-                       "logging": 'on',
-                       "openmp": 'on',
-                       "mpi": 'on',
+                       "openmp": 'off',
+                       "dompi": 'off',
                        "coverage": 'off',
                        "onnxrt": 'off',
                        "cppflow": 'off'}
 
     def requirements(self):
 
-        if self.options.logging == 'on':
-            self.requires("spdlog/1.12.0")
-
         if self.options.cppflow == 'on':
             self.requires("cppflow/2.0.0")
 
@@ -55,23 +50,22 @@ def build_requirements(self):
     def generate(self):
         tc = CMakeToolchain(self)
 
-        tc.variables['docs'] = self.options.docs
-        tc.variables['examples'] = self.options.examples
-        tc.variables['tests'] = self.options.tests
-        tc.variables['benchmarks'] = self.options.benchmarks
-        tc.variables['logging'] = self.options.logging
-        tc.variables['openmp'] = self.options.openmp
-        tc.variables['onnxrt'] = self.options.onnxrt
-        tc.variables['dompi'] = self.options.mpi
-        tc.variables['coverage'] = self.options.coverage
-        tc.variables['cppflow'] = self.options.cppflow
+        tc.cache_variables['docs'] = self.options.docs
+        tc.cache_variables['examples'] = self.options.examples
+        tc.cache_variables['tests'] = self.options.tests
+        tc.cache_variables['benchmarks'] = self.options.benchmarks
+        tc.cache_variables['openmp'] = self.options.openmp
+        tc.cache_variables['onnxrt'] = self.options.onnxrt
+        tc.cache_variables['dompi'] = self.options.dompi
+        tc.cache_variables['coverage'] = self.options.coverage
+        tc.cache_variables['cppflow'] = self.options.cppflow
 
         # List cases where we don't use ccache
         if ('GITHUB_ACTIONS' in os.environ.keys() and self.options.docs == 'off'):
-            tc.variables['CMAKE_C_COMPILER_LAUNCHER'] = "ccache"
-            tc.variables['CMAKE_CXX_COMPILER_LAUNCHER'] = "ccache"
+            tc.cache_variables['CMAKE_C_COMPILER_LAUNCHER'] = "ccache"
+            tc.cache_variables['CMAKE_CXX_COMPILER_LAUNCHER'] = "ccache"
 
-        tc.variables['CMAKE_VERBOSE_MAKEFILE:BOOL'] = "ON"
+        tc.cache_variables['CMAKE_VERBOSE_MAKEFILE:BOOL'] = "ON"
         tc.generate()
 
         deps = CMakeDeps(self)

diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt
@@ -1,15 +1,3 @@
-# Setup logging
-set(SOPT_LOGGER_NAME "sopt" CACHE STRING "NAME of the logger")
-set(SOPT_COLOR_LOGGING true CACHE BOOL "Whether to add color to the log")
-if(logging)
-  set(SOPT_DO_LOGGING 1)
-  set(SOPT_TEST_LOG_LEVEL critical CACHE STRING "Level when logging tests")
-  set_property(CACHE SOPT_TEST_LOG_LEVEL PROPERTY STRINGS
-    off critical error warn info debug trace)
-else()
-  unset(SOPT_DO_LOGGING)
-  set(SOPT_TEST_LOG_LEVEL off)
-endif()
 
 set(version ${SOPT_VERSION})
 string(REGEX REPLACE "\\." ";" version "${SOPT_VERSION}")

diff --git a/cpp/examples/forward_backward/inpainting.cc b/cpp/examples/forward_backward/inpainting.cc
@@ -52,7 +52,6 @@ int main(int argc, char const **argv) {
 
   // Initializes and sets logger (if compiled with logging)
   // See set_level function for levels.
-  sopt::logging::initialize();
   sopt::logging::set_level("debug");
   SOPT_HIGH_LOG("Read input file {}", input);
   Image const image = sopt::notinstalled::read_standard_tiff(input);

diff --git a/cpp/examples/forward_backward/inpainting_credible_interval.cc b/cpp/examples/forward_backward/inpainting_credible_interval.cc
@@ -51,9 +51,7 @@ int main(int argc, char const **argv) {
   std::srand(static_cast<unsigned int>(seed));
   std::mt19937 mersenne(std::time(nullptr));
 
-  // Initializes and sets logger (if compiled with logging)
   // See set_level function for levels.
-  sopt::logging::initialize();
   sopt::logging::set_level("debug");
   SOPT_HIGH_LOG("Read input file {}", input);
   const Image image = sopt::notinstalled::read_standard_tiff(input) /

diff --git a/cpp/examples/forward_backward/inpainting_joint_map.cc b/cpp/examples/forward_backward/inpainting_joint_map.cc
@@ -51,9 +51,7 @@ int main(int argc, char const **argv) {
   std::srand(static_cast<unsigned int>(seed));
   std::mt19937 mersenne(std::time(nullptr));
 
-  // Initializes and sets logger (if compiled with logging)
   // See set_level function for levels.
-  sopt::logging::initialize();
   sopt::logging::set_level("debug");
   SOPT_HIGH_LOG("Read input file {}", input);
   Image const image = sopt::notinstalled::read_standard_tiff(input);

diff --git a/cpp/examples/forward_backward/l2_inpainting.cc b/cpp/examples/forward_backward/l2_inpainting.cc
@@ -4,6 +4,7 @@
 #include <iostream>
 #include <random>
 #include <vector>
+#include <ctime>
 
 #include "sopt/l2_forward_backward.h"
 #include "sopt/logging.h"
@@ -48,9 +49,7 @@ int main(int argc, char const **argv) {
   std::srand(static_cast<unsigned int>(seed));
   std::mt19937 mersenne(std::time(nullptr));
 
-  // Initializes and sets logger (if compiled with logging)
   // See set_level function for levels.
-  sopt::logging::initialize();
   sopt::logging::set_level("debug");
   SOPT_HIGH_LOG("Read input file {}", input);
   Image const image = sopt::notinstalled::read_standard_tiff(input);

diff --git a/cpp/examples/l1_proximal.cc b/cpp/examples/l1_proximal.cc
@@ -3,7 +3,6 @@
 #include "sopt/types.h"
 
 int main(int, char const **) {
-  sopt::logging::initialize();
 
   using Scalar = sopt::t_complex;
   using Real = sopt::real_type<Scalar>::type;