[Solvergraph] Add callback API for node tracking#1890
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a graph tracing mechanism to track the lifetime and state updates of nodes and edges in the solver graph. It replaces direct inheritance from WithUUID in IEdge and INode with a new LifetimeTracker class to safely log creation, destruction, and evaluation events. The reviewer feedback highlights several critical issues: a missing constructor in GraphTracer that could cause compilation failures, a potential MPI race condition when multiple ranks write to the same trace file, and a static initialization order risk. Additionally, the reviewer noted a redundant on_node_evaluate_end function and a potential double-destroy bug on move in LifetimeTracker, suggesting to delete its move operations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| class GraphTracer { | ||
| public: | ||
| std::string filename; | ||
|
|
||
| std::queue<std::string> events; | ||
|
|
||
| public: | ||
| inline void add_event(std::string event) { events.push(event); } | ||
|
|
||
| inline void maybe_flush() { | ||
|
|
||
| // open the file for writing | ||
| std::ofstream file(filename, std::ios::app); | ||
|
|
||
| // lock the file with flock | ||
|
|
||
| // flush the events | ||
| while (!events.empty()) { | ||
| std::string event = events.front(); | ||
| events.pop(); | ||
| file << event << std::endl; | ||
| } | ||
|
|
||
| // close the file | ||
| file.close(); | ||
| } | ||
| }; | ||
|
|
||
| GraphTracer graph_tracer("graph_trace.txt"); |
There was a problem hiding this comment.
Issue: Missing Constructor, MPI Race Condition, and Static Initialization Order Fiasco
- Missing Constructor:
GraphTracerdoes not define a constructor taking astd::string, which will cause a compilation error when initializinggraph_tracer("graph_trace.txt")on compilers that do not support parenthesized aggregate initialization (pre-C++20). - MPI Race Condition: In an MPI-enabled run, every rank will attempt to write to the same file (
graph_trace.txt) concurrently, leading to file corruption or interleaved output. - Static Initialization Order:
graph_traceris initialized as a global variable. Since it may call MPI-related functions or be accessed during static destruction, this can lead to undefined behavior if accessed before MPI is initialized or after global destruction.
Solution
- Add an explicit constructor and destructor to
GraphTracerto ensure safe initialization and automatic flushing of remaining events on destruction. - Use a function-local static
get_graph_tracer()to lazily initialize the tracer after MPI has been initialized, appending the MPI rank to the filename to avoid conflicts. - Update all other occurrences of
graph_tracerin the callback functions to useget_graph_tracer().
class GraphTracer {
public:
std::string filename;
std::queue<std::string> events;
explicit GraphTracer(std::string filename) : filename(std::move(filename)) {}
~GraphTracer() {
maybe_flush();
}
inline void add_event(std::string event) { events.push(event); }
inline void maybe_flush() {
std::ofstream file(filename, std::ios::app);
if (!file) {
return;
}
while (!events.empty()) {
std::string event = events.front();
events.pop();
file << event << std::endl;
}
file.close();
}
};
inline GraphTracer& get_graph_tracer() {
static GraphTracer tracer("graph_trace_" + std::to_string(shamcomm::world_rank()) + ".txt");
return tracer;
}| void on_node_evaluate_end(u64 uuid) { | ||
| graph_tracer.add_event( | ||
| nlohmann::json{ | ||
| {"event", "node_evaluate_end"}, | ||
| {"uuid", uuid}, | ||
| {"wtime", shambase::details::get_wtime()}, | ||
| } | ||
| .dump()); | ||
| graph_tracer.maybe_flush(); | ||
| } |
There was a problem hiding this comment.
Redundant Function
on_node_evaluate_end is redundant because on_node_op already handles op_id == 1 as "node_evaluate_end". Additionally, on_node_evaluate_end is never registered as a callback in init_solver_graph(). Removing it keeps the codebase clean and maintainable.
| LifetimeTracker(LifetimeTracker &&) noexcept = default; | ||
| LifetimeTracker &operator=(LifetimeTracker &&) noexcept = default; |
There was a problem hiding this comment.
Potential Double-Destroy Bug on Move
If a LifetimeTracker is moved, the default move constructor/assignment will copy the uuid member. When the temporary/source LifetimeTracker is destroyed, its destructor will trigger on_destroy with the copied uuid. When the moved-to object is eventually destroyed, it will trigger on_destroy again with the same uuid, leading to a double-destroy notification.
Since LifetimeTracker is managed via std::shared_ptr and should never be moved directly, the move constructor and move assignment operator should be explicitly deleted to prevent accidental moves.
| LifetimeTracker(LifetimeTracker &&) noexcept = default; | |
| LifetimeTracker &operator=(LifetimeTracker &&) noexcept = default; | |
| LifetimeTracker(LifetimeTracker &&) = delete; | |
| LifetimeTracker &operator=(LifetimeTracker &&) = delete; |
There was a problem hiding this comment.
Code Review
This pull request introduces a graph tracing mechanism using a new LifetimeTracker class to monitor the creation, destruction, updates, and evaluation of nodes and edges in the solver graph. The feedback highlights several critical issues and improvement opportunities: resolving potential file write conflicts in parallel MPI environments by using rank-specific filenames, optimizing file I/O performance by keeping the file stream open, deleting move operations in LifetimeTracker to prevent double-destroy notifications, refactoring duplicated edge callback logic, removing dead code, and correcting a comment typo.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| void shammodels::basegodunov::Solver<Tvec, TgridVec>::init_solver_graph() { | ||
|
|
There was a problem hiding this comment.
In a parallel MPI execution, multiple ranks will run concurrently. If all ranks write to the same hardcoded file "graph_trace.txt" without synchronization, the file writes will interleave, leading to corrupted and unparseable JSON data. To prevent this, the filename should be rank-specific, for example by appending shamcomm::world_rank(). Since shamcomm::world_rank() might not be initialized during global static construction, you should initialize or update the filename dynamically inside init_solver_graph().
template<class Tvec, class TgridVec>
void shammodels::basegodunov::Solver<Tvec, TgridVec>::init_solver_graph() {
graph_tracer.filename = "graph_trace_" + std::to_string(shamcomm::world_rank()) + ".txt";| void on_edge_create(u64 uuid) { | ||
| graph_tracer.add_event( | ||
| nlohmann::json{ | ||
| {"event", "edge_create"}, | ||
| {"uuid", uuid}, | ||
| {"wtime", shambase::details::get_wtime()}, | ||
| } | ||
| .dump()); | ||
| } | ||
|
|
||
| void on_edge_destroy(u64 uuid) { | ||
| graph_tracer.add_event( | ||
| nlohmann::json{ | ||
| {"event", "edge_destroy"}, | ||
| {"uuid", uuid}, | ||
| {"wtime", shambase::details::get_wtime()}, | ||
| } | ||
| .dump()); | ||
| } |
There was a problem hiding this comment.
The callbacks on_edge_create and on_edge_destroy share duplicated logic for constructing, dumping, and queuing JSON events, and both lack a call to maybe_flush(). To improve readability and maintainability, refactor this duplicated logic into a helper function or lambda that also ensures maybe_flush() is called.
auto log_event = [&](const std::string& event, u64 uuid) {
graph_tracer.add_event(
nlohmann::json{
{"event", event},
{"uuid", uuid},
{"wtime", shambase::details::get_wtime()},
}
.dump());
graph_tracer.maybe_flush();
};
void on_edge_create(u64 uuid) {
log_event("edge_create", uuid);
}
void on_edge_destroy(u64 uuid) {
log_event("edge_destroy", uuid);
}References
- Refactor duplicated logic into a helper function or lambda to improve readability and maintainability.
| void on_node_evaluate_end(u64 uuid) { | ||
| graph_tracer.add_event( | ||
| nlohmann::json{ | ||
| {"event", "node_evaluate_end"}, | ||
| {"uuid", uuid}, | ||
| {"wtime", shambase::details::get_wtime()}, | ||
| } | ||
| .dump()); | ||
| graph_tracer.maybe_flush(); | ||
| } |
There was a problem hiding this comment.
The function on_node_evaluate_end is defined but never registered as a callback in init_solver_graph(). Instead, on_node_op handles the evaluation end event (when op_id == 1). This function is dead code and should be removed to keep the codebase clean.
| inline void maybe_flush() { | ||
|
|
||
| // open the file for writing | ||
| std::ofstream file(filename, std::ios::app); | ||
|
|
||
| // lock the file with flock | ||
|
|
||
| // flush the events | ||
| while (!events.empty()) { | ||
| std::string event = events.front(); | ||
| events.pop(); | ||
| file << event << std::endl; | ||
| } | ||
|
|
||
| // close the file | ||
| file.close(); | ||
| } |
There was a problem hiding this comment.
Opening and closing the file stream on every single event flush (std::ofstream file(filename, std::ios::app)) introduces a significant performance bottleneck, especially in high-performance simulation codes. Consider keeping the file stream open as a member of GraphTracer (opening it once during initialization and closing it in the destructor), and calling file.flush() or letting the buffer flush naturally to minimize disk I/O overhead.
| LifetimeTracker(LifetimeTracker &&) noexcept = default; | ||
| LifetimeTracker &operator=(LifetimeTracker &&) noexcept = default; |
There was a problem hiding this comment.
Since LifetimeTracker triggers on_destroy in its destructor using its uuid, allowing it to be moved can lead to double-destroy notifications (as both the moved-from temporary and the moved-to destination will share the same uuid and trigger on_destroy upon destruction). Since LifetimeTracker is managed via std::shared_ptr and should never be moved directly, you should explicitly delete its move constructor and move assignment operator to prevent accidental moves.
| LifetimeTracker(LifetimeTracker &&) noexcept = default; | |
| LifetimeTracker &operator=(LifetimeTracker &&) noexcept = default; | |
| LifetimeTracker(LifetimeTracker &&) = delete; | |
| LifetimeTracker &operator=(LifetimeTracker &&) = delete; |
| virtual std::string _impl_get_dot_label() const = 0; | ||
| virtual std::string _impl_get_tex_symbol() const = 0; | ||
|
|
||
| /// Get the UUID of the node |
There was a problem hiding this comment.
The comment says "Get the UUID of the node", but this is the IEdge class. It should say "Get the UUID of the edge".
| /// Get the UUID of the node | |
| /// Get the UUID of the edge |
Workflow reportworkflow report corresponding to commit cc8e495 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportSuggested changesDetailed changes :diff --git a/src/shammodels/ramses/src/Solver.cpp b/src/shammodels/ramses/src/Solver.cpp
index cee99ba3c..ab8e4b353 100644
--- a/src/shammodels/ramses/src/Solver.cpp
+++ b/src/shammodels/ramses/src/Solver.cpp
@@ -276,7 +276,7 @@ class GraphTracer {
std::queue<std::string> events;
public:
- inline void add_event(std::string event) { events.push(event); }
+ inline void add_event(const std::string& event) { events.push(event); }
inline void maybe_flush() {
Detailed changes :+ src/shammodels/ramses/src/Solver.cpp:272: warning: Compound GraphTracer is not documented.
+ src/shammodels/ramses/src/Solver.cpp:274: warning: Member filename (variable) of class GraphTracer is not documented.
+ src/shammodels/ramses/src/Solver.cpp:276: warning: Member events (variable) of class GraphTracer is not documented.
+ src/shammodels/ramses/src/Solver.cpp:279: warning: Member add_event(std::string event) (function) of class GraphTracer is not documented.
+ src/shammodels/ramses/src/Solver.cpp:281: warning: Member maybe_flush() (function) of class GraphTracer is not documented.
+ src/shammodels/ramses/src/Solver.cpp:300: warning: Member graph_tracer("graph_trace.txt") (variable) of file Solver.cpp is not documented.
+ src/shammodels/ramses/src/Solver.cpp:302: warning: Member on_node_create(u64 uuid) (function) of file Solver.cpp is not documented.
+ src/shammodels/ramses/src/Solver.cpp:313: warning: Member on_node_destroy(u64 uuid) (function) of file Solver.cpp is not documented.
+ src/shammodels/ramses/src/Solver.cpp:324: warning: Member on_edge_create(u64 uuid) (function) of file Solver.cpp is not documented.
+ src/shammodels/ramses/src/Solver.cpp:334: warning: Member on_edge_destroy(u64 uuid) (function) of file Solver.cpp is not documented.
+ src/shammodels/ramses/src/Solver.cpp:344: warning: Member on_node_update(shamrock::solvergraph::INode &node) (function) of file Solver.cpp is not documented.
+ src/shammodels/ramses/src/Solver.cpp:371: warning: Member on_node_op(u64 uuid, u64 op_id) (function) of file Solver.cpp is not documented.
+ src/shammodels/ramses/src/Solver.cpp:394: warning: Member on_node_evaluate_end(u64 uuid) (function) of file Solver.cpp is not documented.
- src/shamrock/include/shamrock/solvergraph/IEdge.hpp:28: warning: Compound shamrock::solvergraph::IEdge is not documented.
+ src/shamrock/include/shamrock/solvergraph/IEdge.hpp:30: warning: Compound shamrock::solvergraph::IEdge is not documented.
- src/shamrock/include/shamrock/solvergraph/IEdge.hpp:30: warning: Member get_label() const (function) of class shamrock::solvergraph::IEdge is not documented.
- src/shamrock/include/shamrock/solvergraph/IEdge.hpp:31: warning: Member get_tex_symbol() const (function) of class shamrock::solvergraph::IEdge is not documented.
- src/shamrock/include/shamrock/solvergraph/IEdge.hpp:33: warning: Member _impl_get_dot_label() const =0 (function) of class shamrock::solvergraph::IEdge is not documented.
- src/shamrock/include/shamrock/solvergraph/IEdge.hpp:34: warning: Member _impl_get_tex_symbol() const =0 (function) of class shamrock::solvergraph::IEdge is not documented.
+ src/shamrock/include/shamrock/solvergraph/IEdge.hpp:39: warning: Member get_label() const (function) of class shamrock::solvergraph::IEdge is not documented.
+ src/shamrock/include/shamrock/solvergraph/IEdge.hpp:40: warning: Member get_tex_symbol() const (function) of class shamrock::solvergraph::IEdge is not documented.
+ src/shamrock/include/shamrock/solvergraph/IEdge.hpp:42: warning: Member _impl_get_dot_label() const =0 (function) of class shamrock::solvergraph::IEdge is not documented.
+ src/shamrock/include/shamrock/solvergraph/IEdge.hpp:43: warning: Member _impl_get_tex_symbol() const =0 (function) of class shamrock::solvergraph::IEdge is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:104: warning: Member get_rw_edge_base(int slot) const (function) of class shamrock::solvergraph::INode is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:105: warning: Member get_ro_edge_base(int slot) const (function) of class shamrock::solvergraph::INode is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:114: warning: Member get_rw_edge_base(int slot) const (function) of class shamrock::solvergraph::INode is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:239: warning: Member INODE_DECL_RO(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:240: warning: Member INODE_DECL_RW(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:241: warning: Member INODE_PARAM_RO(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:242: warning: Member INODE_PARAM_RW(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:243: warning: Member INODE_PUSH_RO1(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:244: warning: Member INODE_PUSH_RW1(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:245: warning: Member INODE_PUSH_RO2(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:246: warning: Member INODE_PUSH_RW2(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:247: warning: Member INODE_GET_RO(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:248: warning: Member INODE_GET_RW(type, name) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:250: warning: Member EXPAND_NODE_EDGES(EDGES) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:266: warning: Member INODE_DECL_RO(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:267: warning: Member INODE_DECL_RW(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:268: warning: Member INODE_PARAM_RO(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:269: warning: Member INODE_PARAM_RW(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:270: warning: Member INODE_PUSH_RO1(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:271: warning: Member INODE_PUSH_RW1(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:272: warning: Member INODE_PUSH_RO2(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:273: warning: Member INODE_PUSH_RW2(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:274: warning: Member INODE_GET_RO(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:275: warning: Member INODE_GET_RW(type, name) (macro definition) of file INode.hpp is not documented.
+ src/shamrock/include/shamrock/solvergraph/INode.hpp:277: warning: Member EXPAND_NODE_EDGES(EDGES) (macro definition) of file INode.hpp is not documented.
- src/shamrock/include/shamrock/solvergraph/INode.hpp:95: warning: Member get_ro_edge_base(int slot) const (function) of class shamrock::solvergraph::INode is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:24: warning: Compound shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:26: warning: Member on_create)(u64 uuid) (variable) of class shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:27: warning: Member on_destroy)(u64 uuid) (variable) of class shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:29: warning: Member on_state_update)(T &node) (variable) of class shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:30: warning: Member on_op)(u64 uuid, u64 op_id) (variable) of class shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:41: warning: Member LifetimeTracker(LifetimeTracker &&) noexcept=default (function) of class shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:42: warning: Member operator=(LifetimeTracker &&) noexcept=default (function) of class shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:44: warning: Member notify_update(T &node) (function) of class shamrock::solvergraph::LifetimeTracker is not documented.
+ src/shamrock/include/shamrock/solvergraph/LifetimeTracker.hpp:50: warning: Member notify_op(u64 op_id) (function) of class shamrock::solvergraph::LifetimeTracker is not documented. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
No description provided.