Add celerity blockchain for task divergence checking

[GagaLP]

This pull request adds a divergence checking mechanism for tasks.

It does so by periodically gathering hashes of all tasks from task_recording and comparing them. When a divergence is detected an error containing the diverged tasks and their full task record is printed like:

[2023-10-02 17:31:07.784] [error] Divergence detected in task graph at index 1:
0x471b0f1db5e4b8e6 on nodes 1 0xe9fbff654e3748e1 on nodes 0 [2023-10-02 17:31:07.784] [error] Task record for hash 0x471b0f1db5e4b8e6:
id: 1, debug_name: task_b_4, type: device-compute, cgid: 0
geometry:
dimensions: 2, global_size: [1,1,1], global_offset: [0,0,0], granularity: [1,1,1]
accesses: bid: 0, buffer_name: , mode: R, req: {[64,0,0] - [128,1,1]}
dependencies: node: 0, kind: true-dep, origin: last-epoch

Additionally it also includes a rudimentary deadlock detection for nodes which are stuck by printing a warning after a given amount of time (eg 10 seconds):

[warning] After 10 seconds of waiting nodes 1, did not move to the next task. The runtime might be stuck.

All of this is automatically turned on by running the program with task recording enabled.

Check-perf-impact results: (5a19ced85f862a00d0114dd241122462)

❓ No new benchmark data submitted. ❓
Please re-run the microbenchmarks and include the results if your commit could potentially affect performance.

clang-tidy made some suggestions

clang-tidy made some suggestions

clang-tidy made some suggestions

Regarding the overall design, I mostly like it, but one thing I noticed is that there is a lot of code that's purely for testing which is in the actual "main logic" part. So far, I believe we mostly try to keep testing-only code in testing helpers, which makes it easier to see at a glance what is actually happening in a production run vs. what is done only in testing. We might want to move the testing-related functionality out of the main code.

clang-tidy made some suggestions

LGTM, other than the minor documentation and formatting things discussed offline.

I particularly appreciate moving most of the testing logic and functionality outside the main code path.

Thanks! This is becoming a valuable on-boarding debug feature.

I have not looked closely at the algorithm so far, but some more documentaiton / clarification on what the workflow around the state and the member functions of abstract_block_chain is would help a lot.

Check-perf-impact results: (3b34e58e3c100f4c3541a1ed59580f72)

❓ No new benchmark data submitted. ❓
Please re-run the microbenchmarks and include the results if your commit could potentially affect performance.

clang-tidy made some suggestions

Thanks! I did a first pass and added some comments, mostly about comprehensibility / naming.

Check-perf-impact results: (4c65f1399a47e0eb1340f63004745b17)

❓ No new benchmark data submitted. ❓
Please re-run the microbenchmarks and include the results if your commit could potentially affect performance.

clang-tidy made some suggestions

Things are coming together! GitHub decided to post some of my comments early for some reason... Well, here's the rest :P

By the way, we have a section on diverging execution in ours docs / website, it would be great if you could add a sentence there mentioning this check and how it can be enabled!

I don't understand what this is for..?

The extended_lifetime_hashes are needed, because of how the single node divergence_test_communicator works. The idea is following:

• each simulated node calls allgather sequentially. The send/receive buffer is saved internally as simple pointers.
• When all simulated nodes called the allgather the data is copied.

To prevent those send/receive buffers for per_node_task_hashes from going out of scope we need to "extend" their lifetime. For this, I included the extended_lifetime_hashes vector.

Okay so I took a look at this again and this does not work: divergence_block_chain::collect_hashes returns a per_node_task_hashesby value, which itself contains a std::vector<task_hash>. So extended_lifetime_hashes[0].data() is no longer the same pointer as stored in divergence_test_communicator::m_gather_data, and you're effectively reading already free'd memory. If you run the tests with AddressSanitizier (and without mimalloc), you will get an error for it.

I think the only way the collective operations can be properly mocked (that is, to have them block until all "ranks" have called them), is to put each block chain into a separate thread. The upside is that you won't need any of the pre_check / post_check hackery. I would suggest to abstract all of that into a divergence_check_test_context (which would subsume the divergence_test_communicator_provider) that also wraps a number of divergence_block_chains, each in their own thread, and possibly even the task_test_context for each.

Suggested change

	voidreprot_divergence(constdivergence_map&check_map, constinttask_num);
	voidreport_divergence(constdivergence_map&check_map, constinttask_num);

Comment

Comment

LGTM, other than a few typos and commented out code.

A few typos "Sychronize" "reseting" "shuch"

Also needs to be added to cmake/celerity-config.cmake.in!

Should we print a warning that recording is being force-enabled here? What about the user explicitly setting CELERITY_RECORDING=0? cc @PeterTh@fknorr

Im not a big fan of CELERITY_RECORDING as it exists for that very reason. The user does not care about DAGs being recorded, they care about divergence checks or graph printing, from which we can decide whether recording needs to be active or not.

Agreed, maybe we should just get rid of it in a small follow-up PR.

A few more comments!

Also since the clang-tidy check for this seems broken in CI: Please go over all new function definitions and make sure parameters that can be const are const.

m_is_running must be protected by a mutex!

Or just use an atomic.

Needs a comment on what a true / false return value means. It reads like this would return true when there was divergence, but the function actually throws in that case!

It feels like there is a race between setting m_is_running = true here and the check for m_is_running in run(). I suggest you reverse the order to fix this.

Suggested change

	m_thread=std::thread(&divergence_checker::run, this);
	m_is_running=true;
	m_is_running=true;
	m_thread=std::thread(&divergence_checker::run, this);

Im not a big fan of CELERITY_RECORDING as it exists for that very reason. The user does not care about DAGs being recorded, they care about divergence checks or graph printing, from which we can decide whether recording needs to be active or not.

Comment: What is happening here?

Naming: This does not clear the chain (similar to what vector::clear would do. Maybe something along the lines of erase_front, prune_leading, or similar?

Okay so as discussed offline, we won't include this in 0.5.0 as it needs another revision. The main points:

• Deadlock detection as-is would produce too many false positive warnings; not sure yet how to proceed on this.
• Testing infrastructure invokes UB; needs multi-threading to properly mock blocking collective operations
• We should have a test case (distr_test / integration test?) that exercises the case that one node submits a task while the other does not (the divergence then occurs between that task and the shutdown epoch).