Harden external-QM execution

changes/user/bugfix.external-qm-robustness.md

@@ -0,0 +1 @@
+- External-QM calculations now fail cleanly on process errors or incomplete result files, including when paths contain spaces.

include/QM/external/externalQMRunner.hpp

@@ -49,6 +49,11 @@ namespace QM
             std::string_view script
         ) const;
 
+        virtual void executeCommand(
+            std::string_view command,
+            std::string_view program
+        ) const;
+
        public:
         ExternalQMRunner()           = default;
         ~ExternalQMRunner() override = default;

include/utilities/stringUtilities.hpp

@@ -46,6 +46,8 @@ namespace utilities
     std::string toLowerAndReplaceDashesCopy(std::string);
     std::string toLowerAndReplaceDashesCopy(std::string_view);
     std::string firstLetterToUpperCaseCopy(std::string);
+    std::string shellQuote(std::string_view);
+
     void addSpaces(std::string &, const std::string &, const size_t);
 
     std::uint_fast32_t stringToUintFast32t(const std::string &);

src/QM/external/dftbplusRunner.cpp

@@ -22,8 +22,8 @@
 
 #include "dftbplusRunner.hpp"
 
+#include <cmath>     // for isfinite
 #include <cstddef>   // for size_t
-#include <cstdlib>   // for system
 #include <format>    // for format
 #include <fstream>   // for ofstream
 #include <string>    // for string
@@ -143,11 +143,11 @@ void DFTBPlusRunner::execute()
 
     const auto command = std::format(
         "{} 0 {} 0 0 0 {}",
-        scriptFile,
+        shellQuote(scriptFile),
         reuseCharges,
-        FileSettings::getDFTBFileName()
+        shellQuote(FileSettings::getDFTBFileName())
     );
-    ::system(command.c_str());
+    executeCommand(command, "DFTB+");
 
     _isFirstExecution = false;
 }
@@ -178,9 +178,27 @@ void DFTBPlusRunner::readStressTensor(Box &box, PhysicalData &data)
 
     StaticMatrix3x3<double> stress;
 
-    stressFile >> stress[0][0] >> stress[0][1] >> stress[0][2];
-    stressFile >> stress[1][0] >> stress[1][1] >> stress[1][2];
-    stressFile >> stress[2][0] >> stress[2][1] >> stress[2][2];
+    if (!(stressFile >> stress[0][0] >> stress[0][1] >> stress[0][2] >>
+          stress[1][0] >> stress[1][1] >> stress[1][2] >> stress[2][0] >>
+          stress[2][1] >> stress[2][2]))
+        throw QMRunnerException(
+            std::format(
+                "Incomplete {} stress tensor \"{}\"",
+                string(QMSettings::getQMMethod()),
+                stressFileName
+            )
+        );
+
+    for (size_t row = 0; row < 3; ++row)
+        for (size_t column = 0; column < 3; ++column)
+            if (!std::isfinite(stress[row][column]))
+                throw QMRunnerException(
+                    std::format(
+                        "Invalid value in {} stress tensor \"{}\"",
+                        string(QMSettings::getQMMethod()),
+                        stressFileName
+                    )
+                );
 
     const auto conversion = HARTREE_PER_BOHR3_TO_KCAL_PER_MOL_PER_ANGSTROM3;
     stress                = stress * conversion;

src/QM/external/externalQMRunner.cpp

@@ -23,8 +23,10 @@
 #include "externalQMRunner.hpp"
 
 #include <algorithm>    // for __for_each_fn, for_each
-#include <cmath>        // for isnan, isinf
-#include <filesystem>   // for is_regular_file, path
+#include <array>        // for array
+#include <cmath>        // for isfinite
+#include <cstdlib>      // for system
+#include <filesystem>   // for is_regular_file, path, remove
 #include <format>       // for format
 #include <fstream>      // for ofstream
 #include <string>       // for string
@@ -64,6 +66,13 @@ void ExternalQMRunner::run(SimulationBox &simBox, PhysicalData &physicalData)
 {
     writeCoordsFile(simBox);
 
+    const auto resultFiles = std::array{
+        FileSettings::getQMForcesTempFileName(),
+        FileSettings::getQMChargesTempFileName(),
+        FileSettings::getStressTensorTempFileName()
+    };
+    for (const auto &file : resultFiles) std::filesystem::remove(file);
+
     std::jthread timeoutThread{[this](const std::stop_token stopToken)
                                { throwAfterTimeout(stopToken); }};
 
@@ -91,6 +100,25 @@ std::string ExternalQMRunner::resolveScriptPath(
     return _scriptPath + std::string(script);
 }
 
+void ExternalQMRunner::executeCommand(
+    const std::string_view command,
+    const std::string_view program
+) const
+{
+#if defined(_WIN32)
+    static_cast<void>(command);
+    throw QMRunnerException(
+        std::format("{} command execution is not supported on Windows", program)
+    );
+#else
+    const auto status = std::system(std::string(command).c_str());
+    if (status != EXIT_SUCCESS)
+        throw QMRunnerException(
+            std::format("{} command failed with status {}", program, status)
+        );
+#endif
+}
+
 /**
  * @brief reads the force file (including qm energy) and sets the forces of the
  * atoms
@@ -131,9 +159,16 @@ void ExternalQMRunner::readForceFile(
 
     double energy = 0.0;
 
-    forceFile >> energy;
+    if (!(forceFile >> energy))
+        throw QMRunnerException(
+            std::format(
+                "Cannot read QM energy from {} force file \"{}\"",
+                string(QMSettings::getQMMethod()),
+                forceFileName
+            )
+        );
 
-    if (std::isnan(energy) || std::isinf(energy))
+    if (!std::isfinite(energy))
         throw QMRunnerException(
             std::format(
                 "Invalid QM energy (NaN/Inf) in {} force file \"{}\"",
@@ -148,10 +183,17 @@ void ExternalQMRunner::readForceFile(
     {
         auto grad = linearAlgebra::Vec3D();
 
-        forceFile >> grad[0] >> grad[1] >> grad[2];
+        if (!(forceFile >> grad[0] >> grad[1] >> grad[2]))
+            throw QMRunnerException(
+                std::format(
+                    "Incomplete {} force file \"{}\"",
+                    string(QMSettings::getQMMethod()),
+                    forceFileName
+                )
+            );
 
         for (size_t i = 0; i < 3; ++i)
-            if (std::isnan(grad[i]) || std::isinf(grad[i]))
+            if (!std::isfinite(grad[i]))
                 throw QMRunnerException(
                     std::format(
                         "Invalid QM force component (NaN/Inf) in {} force file "
@@ -207,12 +249,27 @@ void ExternalQMRunner::readChargeFile(SimulationBox &box)
 
     box.resetQMCharges();
 
-    auto readCharges = [&chargeFile](auto &atom)
+    auto readCharges = [&chargeFile, &chargeFileName](auto &atom)
     {
         auto index  = 0;     // Read and discard the first column (index)
         auto charge = 0.0;   // Read the second column (charge value)
 
-        chargeFile >> index >> charge;
+        if (!(chargeFile >> index >> charge))
+            throw QMRunnerException(
+                std::format(
+                    "Incomplete {} charge file \"{}\"",
+                    string(QMSettings::getQMMethod()),
+                    chargeFileName
+                )
+            );
+        if (!std::isfinite(charge))
+            throw QMRunnerException(
+                std::format(
+                    "Invalid value in {} charge file \"{}\"",
+                    string(QMSettings::getQMMethod()),
+                    chargeFileName
+                )
+            );
 
         atom->setQMCharge(charge);
     };

src/QM/external/pyscfRunner.cpp

@@ -22,8 +22,7 @@
 
 #include "pyscfRunner.hpp"
 
-#include <stdlib.h>   // for system, size_t
-
+#include <cstddef>   // for size_t
 #include <format>    // for format
 #include <fstream>   // for ofstream, operator<<, basic_ostream
 #include <string>    // for allocator, string, operator+, operator<<
@@ -84,7 +83,11 @@ void PySCFRunner::execute()
             scriptFileName
         ));
 
-    const auto command = std::format("python {} > pyscf.out", scriptFileName);
+    const auto command = std::format(
+        "python {} > {}",
+        shellQuote(scriptFileName),
+        shellQuote("pyscf.out")
+    );
 
-    ::system(command.c_str());
+    executeCommand(command, "PySCF");
 }

src/QM/external/turbomoleRunner.cpp

@@ -23,7 +23,6 @@
 #include "turbomoleRunner.hpp"
 
 #include <cstddef>   // for size_t
-#include <cstdlib>   // for system
 #include <format>    // for format
 #include <fstream>   // for ofstream
 #include <string>    // for string
@@ -95,8 +94,9 @@ void TurbomoleRunner::execute()
 
     const auto reuseCharges = _isFirstExecution ? 1 : 0;
 
-    const auto command = std::format("{} 0 {} 0 0 0", scriptFile, reuseCharges);
-    ::system(command.c_str());
+    const auto command =
+        std::format("{} 0 {} 0 0 0", shellQuote(scriptFile), reuseCharges);
+    executeCommand(command, "Turbomole");
 
     _isFirstExecution = false;
 }

src/utilities/stringUtilities.cpp

@@ -201,6 +201,29 @@ std::string utilities::firstLetterToUpperCaseCopy(std::string myString)
     return myString;
 }
 
+/**
+ * @brief quotes one argument for a POSIX shell command
+ *
+ * @param argument
+ * @return std::string
+ */
+std::string utilities::shellQuote(const std::string_view argument)
+{
+    std::string quoted{"'"};
+    quoted.reserve(argument.size() + 2);
+
+    for (const auto character : argument)
+    {
+        if (character == '\'')
+            quoted += "'\"'\"'";
+        else
+            quoted += character;
+    }
+
+    quoted += '\'';
+    return quoted;
+}
+
 /**
  * @brief checks if a file exists and can be opened
  *

tests/src/CMakeLists.txt

@@ -1,4 +1,5 @@
 add_subdirectory(setup)
+add_subdirectory(QM)
 add_subdirectory(simulationBox)
 add_subdirectory(linearAlgebra)
 add_subdirectory(manostat)

tests/src/QM/CMakeLists.txt

@@ -0,0 +1,24 @@
+add_executable(testExternalQMRunner
+    testExternalQMRunner.cpp
+)
+
+target_include_directories(testExternalQMRunner
+    PRIVATE
+    ${PROJECT_SOURCE_DIR}/tests/include/macros
+)
+
+target_link_libraries(testExternalQMRunner
+    PRIVATE
+    externalQM
+    gtest
+    gmock
+    pq_test_main
+)
+
+add_test(
+    NAME testExternalQMRunner
+    COMMAND testExternalQMRunner
+    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}/tests
+)
+
+set_property(TEST testExternalQMRunner PROPERTY LABELS QM)