Disable checkpointing properly if openPMD is missing #5493

ikbuibui · 2025-09-29T14:51:04Z

Revert try restart state bug introduced in #5470 #5492 needs to be merged before this.

Moves checkpointing simulation control in its own class
Checkpoint class now has a compile time toggle to disable checkpoint/restart functionality (fixes #5480)
Simplified restart state management and changed from using bools to an enum tracking state

ikbuibui · 2025-09-30T15:03:49Z

include/pmacc/simulationControl/Checkpointing.hpp

+                }
+
+                // Checkpoints are expected to be sorted chronologically.
+                bool const stepFound = std::binary_search(checkpoints.cbegin(), checkpoints.cend(), restartStep);


I have this logic here

Moves checkpointing simulation control in its own class Checkpoint class now has a compile time toggle to disable checkpoint/restart functionality Simplified restart state management and changed from using bools to an enum tracking state

chillenzer

Partial review.

include/picongpu/simulation/control/Simulation.hpp

chillenzer · 2025-11-14T07:22:23Z

include/pmacc/simulationControl/Checkpointing.hpp

+        else
+        {
+            // Handle unknown enum values gracefully.
+            out << "UNKNOWN_RESTART_STATE(" << static_cast<int>(state) << ")";
+        }


Let's hope, we've never gotta do this!

chillenzer · 2025-11-14T07:23:26Z

include/pmacc/simulationControl/Checkpointing.hpp

+        static std::unordered_map<std::string, RestartState> const stringToState
+            = {{"disabled", RestartState::DISABLED},
+               {"try", RestartState::TRY},
+               {"force", RestartState::FORCE},
+               {"success", RestartState::SUCCESS},
+               {"failed", RestartState::FAILED}};


Should we extract this? Having to update this in multiple places seems bound to fail. On the other hand, we'll hopefully not really have to update this ever.

chillenzer · 2025-11-14T07:24:26Z

include/pmacc/simulationControl/Checkpointing.hpp

+            // If the token is not a valid state, set the stream's failbit.
+            // boost::program_options will catch this and report an error to the user.


Wow! Dark magic. Why not just throw yourself here?

I prefer to leave the responsibility of IO to boost po

chillenzer · 2025-11-14T07:28:03Z

include/pmacc/simulationControl/Checkpointing.hpp

+        bool doSoftRestart()
+        {
+            static uint32_t nthSoftRestart = 0;
+            if(nthSoftRestart <= softRestarts)
+            {
+                nthSoftRestart++;
+                return true;
+            }
+            return false;
+        }


Not guarded by checkpointingEnabled.

We allowed restart attempts, even without checkpoints (from the start of the simulation). This only maintains old functionality.

I moved it to this class just to keep the user facing checkpoint.restart.loop input

chillenzer · 2025-11-14T07:31:29Z

include/pmacc/simulationControl/Checkpointing.hpp

+     * @tparam checkpointingEnabled A boolean to enable/disable checkpointing features.
+     */
+    template<bool checkpointingEnabled>
+    struct Checkpointing
+    {


I strongly dislike the approach of guarding each individual function body with an if constexpr because you have to repeat this over and over again. I would have expected to just provide a specialisation

template<> struct Checkpointing<false> { void registerHelp(...) {} ... };

Is there a strong reason for your design?

Used your suggestion

chillenzer · 2025-11-14T07:40:39Z

include/pmacc/simulationControl/Checkpointing.hpp

+                // clang-format off
+                desc.add_options()
+                    ("checkpoint.restart.loop", po::value<uint32_t>(&softRestarts)->default_value(0),
+                    "Number of times to restart the simulation after simulation has finished (for presentations). "
+                    "Note: does not yet work with all plugins, see issue #1305")
+                    ("checkpoint.restart", po::value<RestartState>(&restartState)->zero_tokens()->implicit_value(RestartState::FORCE),
+                     "Restart simulation from a checkpoint. Requires a valid checkpoint.")
+                    ("checkpoint.tryRestart", po::value<RestartState>(&restartState)->zero_tokens()->implicit_value(RestartState::TRY),
+                     "Try to restart if a checkpoint is available else start the simulation from scratch.")
+                    ("checkpoint.restart.directory", po::value<std::string>(&restartDirectory)->default_value(restartDirectory),
+                     "Directory containing checkpoints for a restart")
+                    ("checkpoint.restart.step", po::value<int32_t>(&restartStep),
+                     "Checkpoint step to restart from")
+                    ("checkpoint.period", po::value<std::string>(&checkpointPeriod),
+                     "Period for checkpoint creation [interval(s) based on steps]")
+                    ("checkpoint.timePeriod", po::value<std::uint64_t>(&checkpointPeriodMinutes),
+                     "Time periodic checkpoint creation [period in minutes]")
+                    ("checkpoint.directory", po::value<std::string>(&checkpointDirectory)->default_value(checkpointDirectory),
+                     "Directory for checkpoints");
+                // clang-format on
+                // translate checkpointPeriod string into checkpoint intervals
+                seqCheckpointPeriod = pluginSystem::toTimeSlice(checkpointPeriod);
+            }


Diff'ed against the removed parts from SimulationHelper and looks correct.

chillenzer · 2025-11-14T08:06:56Z

include/pmacc/simulationControl/SimulationHelper.cpp

-        if(checkpointTimeThread.joinable())
-            checkpointTimeThread.join();
-
+        checkpointing.endTimeBasedCheckpointing();


Wow, camel case is not great here. I had to dig into the code to realise that it's not an apocalytic "end-time-based checkpointing" but it's just "ending the time-based checkpointing". Could we rename to "finish"/"cancel"/...?

(I would have been curious what an apocalyptic "end-time-based checkpointing" could look like, though, if you ever want to implement one...)

renamed to finishTimeBasedCheckpointing

chillenzer

Looks okay to me otherwise. Please do the marked renamings.

chillenzer · 2025-11-14T23:00:42Z

include/pmacc/simulationControl/Checkpointing.hpp

+        bool doSoftRestart()
+        {
+            static uint32_t nthSoftRestart = 0;
+            if(nthSoftRestart <= softRestarts)
+            {
+                nthSoftRestart++;
+                return true;
+            }
+            return false;
+        }


Please rename this function. Starting with do... suggests heavily that it actually does something. In the while context it's used in it doesn't read too bad but out of context it's weird. You might want to try something along the lines of moreSoftRestartAttemptsAllowed.

renamed to canSoftRestart. But as I type it now i think this name has the empty problem. It is not clear if the caller asking or telling

now changed to the more verbose tryConsumeRestartAttempt

chillenzer · 2025-11-14T23:01:41Z

include/pmacc/simulationControl/Checkpointing.hpp

+        void doTimeBasedCheckpointing()
+        {
+            if constexpr(checkpointingEnabled)
+            {
+                // register concurrent thread to perform checkpointing periodically after a user defined time
+                if(checkpointPeriodMinutes != 0)
+                    checkpointTimeThread = std::thread(
+                        [&, this]()
+                        {
+                            std::unique_lock<std::mutex> lk(this->concurrentThreadMutex);
+                            while(exitConcurrentThreads.wait_until(
+                                      lk,
+                                      std::chrono::system_clock::now() + std::chrono::minutes(checkpointPeriodMinutes))
+                                  == std::cv_status::timeout)
+                            {
+                                signal::detail::setCreateCheckpoint(1);
+                            }
+                        });
+            }
+        }


Same here: do... suggests that it does something while it actually rather start...s or activate...s.

renamed to startTimeBasedCheckpointing to complement the finishTimeBasedCheckpointing

PrometheusPi · 2025-11-17T10:03:37Z

@ikbuibui what is the status of this PR?

ikbuibui · 2025-11-17T10:18:21Z

I still need to address the requested changes, will happen sometime this week.

Specialized the Checkpointing class template for disabled, i.e. false type Did some renamings Unified the enum to string mapping

ikbuibui commented Sep 30, 2025

View reviewed changes

ikbuibui force-pushed the disable_checkpointing branch from 5b85557 to 24a14ee Compare October 2, 2025 10:49

ikbuibui added the component: core in PIConGPU (core application) label Oct 2, 2025

Make pmacc checkpointing class

6bbde4f

Moves checkpointing simulation control in its own class Checkpoint class now has a compile time toggle to disable checkpoint/restart functionality Simplified restart state management and changed from using bools to an enum tracking state

ikbuibui force-pushed the disable_checkpointing branch from 24a14ee to 6bbde4f Compare October 7, 2025 08:15

ikbuibui changed the title ~~Disable checkpointing~~ Disable checkpointing properly if openPMD is missing Oct 28, 2025

ikbuibui marked this pull request as ready for review October 30, 2025 07:27

ikbuibui requested review from chillenzer and psychocoderHPC October 30, 2025 11:53

chillenzer requested changes Nov 14, 2025

View reviewed changes

Apply review comments

b2f4e59

Specialized the Checkpointing class template for disabled, i.e. false type Did some renamings Unified the enum to string mapping

ikbuibui force-pushed the disable_checkpointing branch from e9e750f to b2f4e59 Compare November 21, 2025 08:38

		// If the token is not a valid state, set the stream's failbit.
		// boost::program_options will catch this and report an error to the user.

Disable checkpointing properly if openPMD is missing #5493

Are you sure you want to change the base?

Disable checkpointing properly if openPMD is missing #5493

Conversation

ikbuibui commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chillenzer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chillenzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PrometheusPi commented Nov 17, 2025

Uh oh!

ikbuibui commented Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ikbuibui commented Sep 29, 2025 •

edited

Loading