Implement state checkpointing for decision nodes by arcondello · Pull Request #587 · dwavesystems/dwave-optimization

arcondello · 2026-07-17T23:22:43Z

Closes #510
Closes #552 by replacing it

AI Generation Disclosure

No AI was used to write the code. I intend to use it to review this PR.

wbernoudy · 2026-07-20T16:54:39Z


+CollectionCheckpoint_::CollectionCheckpoint_(CollectionStateData_* state_ptr) :
+    updates_(),
+    drop_(state_ptr->all_updates_.size()),  // so we ignore any updates added before we're made


What happens if some mutations are made to the node, then you make a checkpoint (causing the drop_ to be set > 0), and then you revert, make more mutations, and commit? Won't this cause some updates to be "dropped" that are actually relevant to the checkpoint?

Actually, I think I mean "make another checkpoint" as final step in the example rather than commit.

Yup! Good catch. Fixed in 214c5e2

This avoids using GCC11 which had some bugs in their ranges implementation.

arcondello · 2026-07-27T18:47:15Z


+    // Overloads required by the DecisionNode ABC *****************************
+
+    // DynamicArrayTestingNode does not impement checkpointing


See #593

Also I see the typo here.

arcondello · 2026-07-27T20:13:26Z

    }
 }

+void Graph::propose(State& state) const {


Could be smarter with #589 or #592, but that's for another PR.

arcondello · 2026-07-27T20:13:49Z

+
+                set_ptr->assign_from_checkpoint(state, std::move(checkpoint0));
+
+                // CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1}));


oops, will restore

arcondello · 2026-07-27T20:14:02Z

+            }
+        }
+
+        // TODO: within one propagation


this is done, need to remove this comment

fastbodin · 2026-07-28T19:37:57Z

+                auto checkpoint = ptr->checkpoint(state);  // 0, 1, 0, 1, 0, 1, 0, 1, 0, 1
+
+                THEN("We can mutate and then assign from that checkpoint") {
+                    ptr->set_value(state, 0, 1);  // 0, 1, 0, 1, 0, 1, 0, 1, 0, 1


// 1, 1, 0, 1, 0, 1, 0, 1, 0, 1

since you called set_value()

fastbodin · 2026-07-28T19:41:06Z

+                ptr->exchange(state, 0, 2);   // [-2, -4, -4, -2, 0, 0, 2, 2, 4, 4]
+                ptr->set_value(state, 3, 1);  // [-2, -4, -4, 1, 0, 0, 2, 2, 4, 4]
+
+                THEN("We can commit, then assign from the checkpoint") {


Nit:

THEN("We can propose, then assign from the checkpoint") {

I realize propose is propagate and commit. As this is a Nit, I am not going to comment at all duplicate instances.

fastbodin · 2026-07-28T19:41:45Z

+            AND_WHEN("We create a checkpoint to that state") {
+                auto checkpoint = ptr->checkpoint(state);  // 0, 1, 0, 1, 0, 1, 0, 1, 0, 1
+
+                THEN("We can mutate and then assign from that checkpoint") {


Nit

THEN("We can mutate, then propose, and then assign from that checkpoint") {

fastbodin · 2026-07-28T23:11:00Z

+                        CHECK_THAT(inode_ptr->view(state), RangeEquals({8, 8, 8, 8, 8, 8, -3, 3}));
+                    }
+                }
+            }


A bit of a pain, but I think it might be worth adding a single test for BinaryNode with checkpointing to check that all the correct indices are cached with 1s and 0s (I am referring to the BinaryNode DisjointSparseSet).

THEN("Sum constraint sums, tracked indices, and state are correct") { CHECK(bnode_ptr->sum_constraints_lhs(state).size() == 1); CHECK(bnode_ptr->sum_constraints_lhs(state).data()[0].size() == 1); CHECK_THAT(bnode_ptr->sum_constraints_lhs(state)[0], RangeEquals({3})); check_indices<true>(state, bnode_ptr, 0, 0, {1, 2, 5}); // <- ***** This check is the important one ****** check_indices<false>(state, bnode_ptr, 0, 0, {0, 3, 4}); // <- ***** And this one ****** CHECK_THAT(bnode_ptr->view(state), RangeEquals(expected_init)); }

fastbodin · 2026-07-28T23:31:49Z

+            const auto& [idx, old, _] :
+            state_data->diff() | std::views::reverse | std::views::take(excess_updates)
+        ) {
+            state_data->set(idx, old);


I would just add a note that this by-passes (currently) the sum constraint lhs tracking.

Looking at this again, can you remind we why this is fine?

fastbodin · 2026-07-28T23:40:48Z

        assert(size_ >= 0 && static_cast<std::size_t>(size_) == buffer.size());
    }

+    // Commit the changes and clear the diff by returning the diff buffer.


Confused where the commit() is and am feeling like I am missing something.

Commiting the changes just means keeping the status quo 😄

Wait. Given a committed state, if we then make a series of changes (and create a diff) this method detaches the diff. Is this considered a silent commit()? The state is still modified but now we don't have the diff to now how it was modified?

fastbodin · 2026-07-28T23:42:26Z

        size_ = buffer.size();
    }

+    // Commit the changes and clear the diff by returning the diff buffer.


Same doc string comments as commit_and_detach(). Intentional?

Should say "revert"

fastbodin · 2026-07-30T21:11:51Z

+    if (auto* prev_ptr = static_cast<DiffCheckpoint*>(prev_ptr_)) {
+        prev_ptr->commit_updates(std::vector<Update>(diff.begin(), diff.end()));
+        assert(prev_ptr->drop_ == 0);
+    }


To be clear, we are doing the static_cast because the prev_ptr_ member is a LinkedListCheckpoint which does not have the method commit_updates(). Functionally, the only reason the static_cast would return a nullptr is if prev_ptr_ is a nullptr already.

I am just comparing this to the destructor which checks if (prev_ptr_ == nullptr) and does not have a if around static_cast<DiffCheckpoint*>(prev_ptr_). It feels like this should either be

if (auto* prev_ptr = static_cast<DiffCheckpoint*>(prev_ptr_)) { prev_ptr->commit_updates(std::vector<Update>(diff.begin(), diff.end())); assert(prev_ptr->drop_ == 0); return; } assert(prev_ptr_ == nullptr);

or should resemble the destructor

if (prev_ptr_ == nullptr) return; auto* prev_ptr = static_cast<DiffCheckpoint*>(prev_ptr_); assert(prev_ptr); // <- I added this prev_ptr->commit_updates(std::vector<Update>(diff.begin(), diff.end())); assert(prev_ptr->drop_ == 0);

fastbodin · 2026-07-30T21:12:39Z

+    if (prev_ptr_ == nullptr) return;
+
+    // otherwise we need to transfer our info over
+    auto* prev_ptr = static_cast<DiffCheckpoint*>(prev_ptr_);


See comment above. If we keep this structure, I would consider adding

assert(prev_ptr);

fastbodin · 2026-07-30T21:16:43Z

+    // otherwise we need to transfer our info over
+    auto* prev_ptr = static_cast<DiffCheckpoint*>(prev_ptr_);
+    assert(prev_ptr->drop_ == 0);
+    for (auto& updates : updates_) prev_ptr->commit_updates(std::move(updates));


The reason we do not do

prev_ptr->commit_updates(this->detach_updates());

is because drop_ (which I would love a doc string for 😄) says how many of the vectors in vector<vector<Updates>> we drop?

how many updates of the last vector. And agree drop_ needs a docstring, @alexzucca90 had the same feedback

The last vector! Okay. In that case, why do the loop as opposed to

prev_ptr->commit_updates(this->detach_updates());

prev_ptr->commit_updates() takes a single diff (usually from the node state). Here we want to transfer a sequence of diffs. We could flatten, or add an overload I suppose..

It does not make a difference really to me. Just trying to understand. I had thought that

auto detach_updates() { auto updates = std::move(updates_) | std::views::join; assert(updates_.empty()); return updates; }

would result in a std::vector<Update> since updates_ is a std::vector<std::vector<Update>>. I had not thought you would need an overload?

arcondello · 2026-07-30T21:30:30Z

I think I need to take another documentation pass. I'll try to do that today

fastbodin

Officially gone through everything. I need to look at the number.cpp and collections.cpp again. On the whole, looks good. Most of my comments above are clarifications. I will resolved them ASAP to avoid clogging the thread on this PR.

fastbodin · 2026-07-30T22:11:30Z

+        // Right now, you can only revert to the most recent checkpoint. It's
+        // pretty straightforward to support going further back, but this is all
+        // we need right now.
+        assert(this->checkpoint_ptr<CollectionCheckpoint_>() == checkpoint_ptr);


Took me a hot minute to realize this was templated from

class CheckpointableState { protected: template <std::derived_from<LinkedListCheckpoint> T> T* checkpoint_ptr() { return static_cast<T*>(prev_ptr_); }

fastbodin · 2026-07-30T22:25:12Z

+// DisjointBitSetsNode is on the way out, so let's do the simplest possible
+// implementation for now.


fastbodin · 2026-07-30T22:25:47Z

+    State& state,
+    std::unique_ptr<NodeStateCheckpoint>& checkpoint
+) const {
+    const DisjointBitSetsCheckpoint_* checkpoint_ptr =


const auto*?

fastbodin · 2026-07-30T22:40:51Z

+                        check.reset();
+                    }
+                }
+            }


I love this test and wish there was a final CHECK() we could do :rofl

fastbodin · 2026-07-30T22:46:55Z

+
+    auto* checkpoint_ptr = static_cast<NumberNodeCheckpoint_*>(checkpoint.get());
+
+    // todo: assert that this checkpoint is the latest


Testing my understanding. You mean

// Right now, you can only revert to the most recent checkpoint. It's // pretty straightforward to support going further back, but this is all // we need right now. assert(this->checkpoint_ptr<NumberNodeCheckpoint_>() == checkpoint_ptr);

fastbodin · 2026-07-30T22:48:04Z

+            const auto& [idx, old, _] :
+            state_data->diff() | std::views::reverse | std::views::take(excess_updates)
+        ) {
+            state_data->set(idx, old);


Looking at this again, can you remind we why this is fine?

arcondello added the enhancement New feature or request label Jul 17, 2026

Implement state checkpointing for CollectionNode

953304e

arcondello force-pushed the feature/decision-checkpoints branch from 6df7006 to 953304e Compare July 17, 2026 23:25

wbernoudy reviewed Jul 20, 2026

View reviewed changes

Fix reverts and dangling pointers for CollectionNode checkpoints

214c5e2

arcondello force-pushed the feature/decision-checkpoints branch 2 times, most recently from b4679c2 to 214c5e2 Compare July 20, 2026 18:59

Use offical Python images in CircleCI

9cd035b

This avoids using GCC11 which had some bugs in their ranges implementation.

arcondello force-pushed the feature/decision-checkpoints branch from 86b97e8 to 9cd035b Compare July 20, 2026 19:18

arcondello commented Jul 20, 2026

View reviewed changes

Comment thread dwave/optimization/include/dwave-optimization/state.hpp Outdated

arcondello added 4 commits July 20, 2026 15:05

Add LinkedListCheckpoint helper class

6ec49f9

Fix the copying of checkpointed states

fc44824

Drop unused NodeStateCheckpoint::valid() method

f711e63

Add DiffCheckpoint class for checkpoints that track diffs

e85f85f

arcondello force-pushed the feature/decision-checkpoints branch 2 times, most recently from f1aa563 to 06b9dc5 Compare July 21, 2026 21:20

Go back to using CircleCI image for some CI tasks

f70699e

arcondello force-pushed the feature/decision-checkpoints branch from 06b9dc5 to f70699e Compare July 21, 2026 21:21

arcondello added 3 commits July 23, 2026 15:28

Add NumberNode::checkpoint()

ef950e4

Fix assigning from a checkpoint after mutation

e61a70f

Add test for BinaryNode::assign_from_checkpoint

e845381

arcondello force-pushed the feature/decision-checkpoints branch from 30637e6 to 5fbe15b Compare July 27, 2026 18:46

arcondello changed the title ~~Implement state checkpointing for CollectionNode~~ Implement state checkpointing for decision nodes Jul 27, 2026

arcondello commented Jul 27, 2026

View reviewed changes

Make all DecisionNodes implement checkpointing

4402568

arcondello force-pushed the feature/decision-checkpoints branch from 5fbe15b to 4402568 Compare July 27, 2026 18:51

arcondello marked this pull request as ready for review July 27, 2026 18:51

arcondello requested a review from fastbodin July 27, 2026 18:52

arcondello commented Jul 27, 2026

View reviewed changes

fastbodin reviewed Jul 29, 2026

View reviewed changes

fastbodin reviewed Jul 30, 2026

View reviewed changes


		// Overloads required by the DecisionNode ABC *****************************

		// DynamicArrayTestingNode does not impement checkpointing


		set_ptr->assign_from_checkpoint(state, std::move(checkpoint0));

		// CHECK_THAT(set_ptr->view(state), RangeEquals({0, 1}));

		// DisjointBitSetsNode is on the way out, so let's do the simplest possible
		// implementation for now.


		auto* checkpoint_ptr = static_cast<NumberNodeCheckpoint_*>(checkpoint.get());

		// todo: assert that this checkpoint is the latest

Uh oh!

Conversation

arcondello commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Generation Disclosure

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcondello Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arcondello Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fastbodin Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

arcondello commented Jul 30, 2026

Uh oh!

fastbodin 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!

arcondello commented Jul 17, 2026 •

edited

Loading

arcondello Jul 20, 2026 •

edited

Loading

arcondello Jul 27, 2026 •

edited

Loading

fastbodin Jul 30, 2026 •

edited

Loading