Merge pull request #6478 from wthrowe/checkpoint_test_failure

nilsvu · web-flow · commit 8522bfbef6c8 · 2025-02-07T19:12:19.000-08:00
diff --git a/cmake/RunInputFileTest.sh b/cmake/RunInputFileTest.sh
@@ -35,6 +35,7 @@ for expected_code in $4 ; do
         restart=$(expr $restart + 1)
     fi
     if [ $exit_code -ne $expected_code ]; then
+        echo "ERROR: Exited with ${exit_code} instead of ${expected_code}" >&2
         exit 1
     fi
 done
diff --git a/src/Parallel/CMakeLists.txt b/src/Parallel/CMakeLists.txt
@@ -13,6 +13,7 @@ spectre_target_sources(
   PRIVATE
   ArrayComponentId.cpp
   CharmRegistration.cpp
+  ExitCode.cpp
   InitializationFunctions.cpp
   NodeLock.cpp
   Phase.cpp
diff --git a/src/Parallel/ExitCode.cpp b/src/Parallel/ExitCode.cpp
@@ -0,0 +1,26 @@
+// Distributed under the MIT License.
+// See LICENSE.txt for details.
+
+#include "Parallel/ExitCode.hpp"
+
+#include <ostream>
+#include <type_traits>
+
+#include "Utilities/ErrorHandling/Error.hpp"
+
+namespace Parallel {
+std::ostream& operator<<(std::ostream& os, const ExitCode& code) {
+  os << static_cast<std::underlying_type_t<ExitCode>>(code);
+  switch (code) {
+    case Parallel::ExitCode::Complete:
+      return os << " (Complete)";
+    case Parallel::ExitCode::Abort:
+      return os << " (Abort)";
+    case Parallel::ExitCode::ContinueFromCheckpoint:
+      return os << " (ContinueFromCheckpoint)";
+    default:
+      ERROR("Unknown exit code: "
+            << static_cast<std::underlying_type_t<ExitCode>>(code));
+  }
+}
+}  // namespace Parallel
diff --git a/src/Parallel/ExitCode.hpp b/src/Parallel/ExitCode.hpp
@@ -3,6 +3,8 @@
 
 #pragma once
 
+#include <iosfwd>
+
 #include "DataStructures/DataBox/Tag.hpp"
 
 namespace Parallel {
@@ -23,6 +25,8 @@ enum class ExitCode : int {
   ContinueFromCheckpoint = 2
 };
 
+std::ostream& operator<<(std::ostream& os, const ExitCode& code);
+
 namespace Tags {
 
 /*!
diff --git a/src/Parallel/PhaseControl/CheckpointAndExitAfterWallclock.cpp b/src/Parallel/PhaseControl/CheckpointAndExitAfterWallclock.cpp
@@ -21,15 +21,6 @@ std::optional<Parallel::Phase> RestartPhase::combine_method::operator()(
       "arbitration in the Main chare, so no reduction data should be "
       "provided.");
 }
-
-std::optional<double> WallclockHoursAtCheckpoint::combine_method::operator()(
-    const std::optional<double> /*first_time*/,
-    const std::optional<double>& /*second_time*/) {
-  ERROR(
-      "The wallclock time at which a checkpoint was requested should "
-      "only be altered by the phase change arbitration in the Main "
-      "chare, so no reduction data should be provided.");
-}
 }  // namespace Tags
 
 CheckpointAndExitAfterWallclock::CheckpointAndExitAfterWallclock(
diff --git a/src/Parallel/PhaseControl/CheckpointAndExitAfterWallclock.hpp b/src/Parallel/PhaseControl/CheckpointAndExitAfterWallclock.hpp
@@ -46,22 +46,6 @@ struct RestartPhase {
   using main_combine_method = combine_method;
 };
 
-/// Storage in the phase change decision tuple so that the Main chare can record
-/// the elapsed wallclock time since the start of the run.
-///
-/// \note This tag is not intended to participate in any of the reduction
-/// procedures, so will error if the combine method is called.
-struct WallclockHoursAtCheckpoint {
-  using type = std::optional<double>;
-
-  struct combine_method {
-    [[noreturn]] std::optional<double> operator()(
-        const std::optional<double> /*first_time*/,
-        const std::optional<double>& /*second_time*/);
-  };
-  using main_combine_method = combine_method;
-};
-
 /// Stores whether the checkpoint and exit has been requested.
 ///
 /// Combinations are performed via `funcl::Or`, as the phase in question should
@@ -146,8 +130,7 @@ struct CheckpointAndExitAfterWallclock : public PhaseChange {
   using return_tags = tmpl::list<>;
 
   using phase_change_tags_and_combines =
-      tmpl::list<Tags::RestartPhase, Tags::WallclockHoursAtCheckpoint,
-                 Tags::CheckpointAndExitRequested>;
+      tmpl::list<Tags::RestartPhase, Tags::CheckpointAndExitRequested>;
 
   template <typename Metavariables>
   using participating_components = typename Metavariables::component_list;
@@ -174,15 +157,22 @@ struct CheckpointAndExitAfterWallclock : public PhaseChange {
 
  private:
   std::optional<double> wallclock_hours_for_checkpoint_and_exit_ = std::nullopt;
+  // This flag is set during arbitration when the class decides to
+  // halt the run.  As it is not checkpointed, this distinguishes the
+  // state immediately after writing the checkpoint from that
+  // immediately after reading it during the restart.
+  //
+  // Phase arbitration is only run from Main, so there are no
+  // threading issues here.
+  // NOLINTNEXTLINE(spectre-mutable)
+  mutable bool halting_ = false;
 };
 
 template <typename... DecisionTags>
 void CheckpointAndExitAfterWallclock::initialize_phase_data_impl(
     const gsl::not_null<tuples::TaggedTuple<DecisionTags...>*>
         phase_change_decision_data) const {
   tuples::get<Tags::RestartPhase>(*phase_change_decision_data) = std::nullopt;
-  tuples::get<Tags::WallclockHoursAtCheckpoint>(*phase_change_decision_data) =
-      std::nullopt;
   tuples::get<Tags::CheckpointAndExitRequested>(*phase_change_decision_data) =
       false;
 }
@@ -214,21 +204,12 @@ CheckpointAndExitAfterWallclock::arbitrate_phase_change_impl(
 
   auto& restart_phase =
       tuples::get<Tags::RestartPhase>(*phase_change_decision_data);
-  auto& wallclock_hours_at_checkpoint =
-      tuples::get<Tags::WallclockHoursAtCheckpoint>(
-          *phase_change_decision_data);
   auto& exit_code =
       tuples::get<Parallel::Tags::ExitCode>(*phase_change_decision_data);
   if (restart_phase.has_value()) {
-    ASSERT(wallclock_hours_at_checkpoint.has_value(),
-           "Consistency error: Should have recorded the Wallclock time "
-           "while recording a phase to restart from.");
     // This `if` branch, where restart_phase has a value, is the
-    // post-checkpoint call to arbitrate_phase_change. Depending on the time
-    // elapsed so far in this run, next phase is...
-    // - Exit, if the time is large
-    // - restart_phase, if the time is small
-    if (elapsed_hours >= wallclock_hours_at_checkpoint.value()) {
+    // post-checkpoint call to arbitrate_phase_change.
+    if (halting_) {
       // Preserve restart_phase for use after restarting from the checkpoint
       exit_code = Parallel::ExitCode::ContinueFromCheckpoint;
       return std::make_pair(Parallel::Phase::Exit,
@@ -245,7 +226,6 @@ CheckpointAndExitAfterWallclock::arbitrate_phase_change_impl(
       // Reset restart_phase until it is needed for the next checkpoint
       const auto result = restart_phase;
       restart_phase.reset();
-      wallclock_hours_at_checkpoint.reset();
       return std::make_pair(result.value(),
                             ArbitrationStrategy::PermitAdditionalJumps);
     }
@@ -260,7 +240,8 @@ CheckpointAndExitAfterWallclock::arbitrate_phase_change_impl(
                              std::numeric_limits<double>::infinity())) {
       // Record phase and actual elapsed time for determining following phase
       restart_phase = current_phase;
-      wallclock_hours_at_checkpoint = elapsed_hours;
+      ASSERT(not halting_, "Halting for checkpoint recursively");
+      halting_ = true;
       return std::make_pair(Parallel::Phase::WriteCheckpoint,
                             ArbitrationStrategy::RunPhaseImmediately);
     }
diff --git a/tests/Unit/Parallel/CMakeLists.txt b/tests/Unit/Parallel/CMakeLists.txt
@@ -165,6 +165,7 @@ set(LIBRARY "Test_Parallel")
 set(LIBRARY_SOURCES
   Test_ArrayComponentId.cpp
   Test_DomainDiagnosticInfo.cpp
+  Test_ExitCode.cpp
   Test_GlobalCacheDataBox.cpp
   Test_InboxInserters.cpp
   Test_MemoryMonitor.cpp
diff --git a/tests/Unit/Parallel/PhaseControl/Test_CheckpointAndExitAfterWallclock.cpp b/tests/Unit/Parallel/PhaseControl/Test_CheckpointAndExitAfterWallclock.cpp
@@ -8,6 +8,7 @@
 
 #include "DataStructures/DataBox/DataBox.hpp"
 #include "Framework/TestCreation.hpp"
+#include "Framework/TestHelpers.hpp"
 #include "Options/Protocols/FactoryCreation.hpp"
 #include "Parallel/ExitCode.hpp"
 #include "Parallel/GlobalCache.hpp"
@@ -56,14 +57,12 @@ SPECTRE_TEST_CASE("Unit.Parallel.PhaseControl.CheckpointAndExitAfterWallclock",
   {
     INFO("Test initialize phase change decision data");
     PhaseChangeDecisionData phase_change_decision_data{
-        Parallel::Phase::Execute, true, 1.0, true,
-        Parallel::ExitCode::Complete};
+        Parallel::Phase::Execute, true, true, Parallel::ExitCode::Complete};
     phase_change0.initialize_phase_data<Metavariables>(
         make_not_null(&phase_change_decision_data));
-    // extra parens in the check prevent Catch from trying to stream the tuple
-    CHECK((phase_change_decision_data ==
-           PhaseChangeDecisionData{std::nullopt, std::nullopt, false, true,
-                                   Parallel::ExitCode::Complete}));
+    CHECK(phase_change_decision_data ==
+          PhaseChangeDecisionData{std::nullopt, false, true,
+                                  Parallel::ExitCode::Complete});
   }
   {
     INFO("Wallclock time < big trigger time");
@@ -72,93 +71,76 @@ SPECTRE_TEST_CASE("Unit.Parallel.PhaseControl.CheckpointAndExitAfterWallclock",
     // the PhaseChange with a big trigger time.
     // (this assumes the test doesn't take 1h to get here)
     PhaseChangeDecisionData phase_change_decision_data{
-        std::nullopt, std::nullopt, true, true, Parallel::ExitCode::Complete};
+        std::nullopt, true, true, Parallel::ExitCode::Complete};
     const auto decision_result = phase_change1.arbitrate_phase_change(
         make_not_null(&phase_change_decision_data), Parallel::Phase::Execute,
         cache);
-    CHECK((decision_result == std::nullopt));
-    CHECK((phase_change_decision_data ==
-           PhaseChangeDecisionData{std::nullopt, std::nullopt, false, true,
-                                   Parallel::ExitCode::Complete}));
+    CHECK(decision_result == std::nullopt);
+    CHECK(phase_change_decision_data ==
+          PhaseChangeDecisionData{std::nullopt, false, true,
+                                  Parallel::ExitCode::Complete});
   }
   {
     INFO("Wallclock time > small trigger time");
     // Now check case where wallclock time > trigger wallclock time, using
     // the PhaseChange with a tiny trigger time.
     // (this assumes the test takes at least a few cycles to get here)
     PhaseChangeDecisionData phase_change_decision_data{
-        std::nullopt, std::nullopt, true, true, Parallel::ExitCode::Complete};
+        std::nullopt, true, true, Parallel::ExitCode::Complete};
     const auto decision_result = phase_change0.arbitrate_phase_change(
         make_not_null(&phase_change_decision_data), Parallel::Phase::Execute,
         cache);
-    CHECK((decision_result ==
-           std::make_pair(
-               Parallel::Phase::WriteCheckpoint,
-               PhaseControl::ArbitrationStrategy::RunPhaseImmediately)));
-    // It's impossible to know what the elapsed wallclock time will be, so we
-    // check the tags one by one...
-    CHECK((tuples::get<PhaseControl::Tags::RestartPhase>(
-               phase_change_decision_data) == Parallel::Phase::Execute));
-    // Check recorded time in range: 0 second < time < 1 second
-    // (this assumes test run duration falls in this time window)
-    CHECK(tuples::get<PhaseControl::Tags::WallclockHoursAtCheckpoint>(
-              phase_change_decision_data) > 0.0);
-    const double one_second = 1.0 / 3600.0;
-    CHECK(tuples::get<PhaseControl::Tags::WallclockHoursAtCheckpoint>(
-              phase_change_decision_data) < one_second);
-    CHECK(tuples::get<PhaseControl::Tags::CheckpointAndExitRequested>(
-              phase_change_decision_data) == false);
+    CHECK(
+        decision_result ==
+        std::make_pair(Parallel::Phase::WriteCheckpoint,
+                       PhaseControl::ArbitrationStrategy::RunPhaseImmediately));
+    CHECK(phase_change_decision_data ==
+          PhaseChangeDecisionData{Parallel::Phase::Execute, false, true,
+                                  Parallel::ExitCode::Complete});
   }
   {
     INFO("Restarting from checkpoint");
     // Check behavior following the checkpoint phase
-    // First check case where wallclock time < recorded time, which corresponds
-    // to restarting from a checkpoint. Should update options next.
-    // (this assumes the test doesn't take 1h to get here)
+    const PhaseControl::CheckpointAndExitAfterWallclock phase_change_restart =
+        serialize_and_deserialize(phase_change0);
     PhaseChangeDecisionData phase_change_decision_data{
-        Parallel::Phase::Execute, 1.0, false, true,
-        Parallel::ExitCode::Complete};
-    auto decision_result = phase_change0.arbitrate_phase_change(
+        Parallel::Phase::Execute, false, true, Parallel::ExitCode::Complete};
+    auto decision_result = phase_change_restart.arbitrate_phase_change(
         make_not_null(&phase_change_decision_data),
         Parallel::Phase::WriteCheckpoint, cache);
-    CHECK((decision_result ==
-           std::make_pair(
-               Parallel::Phase::UpdateOptionsAtRestartFromCheckpoint,
-               PhaseControl::ArbitrationStrategy::PermitAdditionalJumps)));
-    CHECK((phase_change_decision_data ==
-           PhaseChangeDecisionData{Parallel::Phase::Execute, 1.0, false, true,
-                                   Parallel::ExitCode::Complete}));
+    CHECK(decision_result ==
+          std::make_pair(
+              Parallel::Phase::UpdateOptionsAtRestartFromCheckpoint,
+              PhaseControl::ArbitrationStrategy::PermitAdditionalJumps));
+    CHECK(phase_change_decision_data ==
+          PhaseChangeDecisionData{Parallel::Phase::Execute, false, true,
+                                  Parallel::ExitCode::Complete});
 
     // Now, from update phase, go back to Execute
-    decision_result = phase_change0.arbitrate_phase_change(
+    decision_result = phase_change_restart.arbitrate_phase_change(
         make_not_null(&phase_change_decision_data),
         Parallel::Phase::UpdateOptionsAtRestartFromCheckpoint, cache);
-    CHECK((decision_result ==
-           std::make_pair(
-               Parallel::Phase::Execute,
-               PhaseControl::ArbitrationStrategy::PermitAdditionalJumps)));
-    CHECK((phase_change_decision_data ==
-           PhaseChangeDecisionData{std::nullopt, std::nullopt, false, true,
-                                   Parallel::ExitCode::Complete}));
+    CHECK(decision_result ==
+          std::make_pair(
+              Parallel::Phase::Execute,
+              PhaseControl::ArbitrationStrategy::PermitAdditionalJumps));
+    CHECK(phase_change_decision_data ==
+          PhaseChangeDecisionData{std::nullopt, false, true,
+                                  Parallel::ExitCode::Complete});
   }
   {
     INFO("Exiting after checkpoint");
-    // Now check case where wallclock time > recorded time, which corresponds to
-    // having just written a checkpoint. We want to exit with exit code 2 now.
-    // (this assumes the test takes at least a few cycles to get here)
     PhaseChangeDecisionData phase_change_decision_data{
-        Parallel::Phase::Execute, 1e-15, false, true,
-        Parallel::ExitCode::Complete};
+        Parallel::Phase::Execute, false, true, Parallel::ExitCode::Complete};
     const auto decision_result = phase_change0.arbitrate_phase_change(
         make_not_null(&phase_change_decision_data),
         Parallel::Phase::WriteCheckpoint, cache);
-    CHECK((decision_result ==
-           std::make_pair(
-               Parallel::Phase::Exit,
-               PhaseControl::ArbitrationStrategy::RunPhaseImmediately)));
     CHECK(
-        (phase_change_decision_data ==
-         PhaseChangeDecisionData{Parallel::Phase::Execute, 1e-15, false, true,
-                                 Parallel::ExitCode::ContinueFromCheckpoint}));
+        decision_result ==
+        std::make_pair(Parallel::Phase::Exit,
+                       PhaseControl::ArbitrationStrategy::RunPhaseImmediately));
+    CHECK(phase_change_decision_data ==
+          PhaseChangeDecisionData{Parallel::Phase::Execute, false, true,
+                                  Parallel::ExitCode::ContinueFromCheckpoint});
   }
 }
diff --git a/tests/Unit/Parallel/Test_ExitCode.cpp b/tests/Unit/Parallel/Test_ExitCode.cpp
@@ -0,0 +1,16 @@
+// Distributed under the MIT License.
+// See LICENSE.txt for details.
+
+#include "Framework/TestingFramework.hpp"
+
+#include "Parallel/ExitCode.hpp"
+#include "Utilities/GetOutput.hpp"
+
+SPECTRE_TEST_CASE("Unit.Parallel.ExitCode", "[Parallel][Unit]") {
+  CHECK(get_output(Parallel::ExitCode::Complete) == "0 (Complete)");
+  CHECK(get_output(Parallel::ExitCode::Abort) == "1 (Abort)");
+  CHECK(get_output(Parallel::ExitCode::ContinueFromCheckpoint) ==
+        "2 (ContinueFromCheckpoint)");
+  CHECK_THROWS_WITH(get_output(static_cast<Parallel::ExitCode>(3)),
+                    Catch::Matchers::ContainsSubstring("Unknown exit code: 3"));
+}
