Conversation
CactusGraph::load_weights GraphFile::load_into_graph MappedFile::load_into_graph mmap_load Signed-off-by: Noah Cylich <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR removes unused graph weight/tensor loading APIs and updates the graph I/O test to use the remaining mmap-based loading path.
Changes:
- Removed
CactusGraph::load_weightsfrom the publicCactusGraphAPI. - Removed
GraphFile::load_into_graph,GraphFile::mmap_load, andGraphFile::MappedFile::load_into_graphimplementations/declarations. - Updated
tests/test_graph.cppto useCactusGraph::mmap_weights()+get_output_buffer()for validation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_graph.cpp | Switches the save/load test to validate mmap-based weight loading via returned node id and get_output_buffer(). |
| cactus/graph/graph_io.cpp | Deletes unused file-loading helpers and mmap wrapper, leaving MappedFile + graph mmap entrypoints as the supported path. |
| cactus/graph/graph.h | Removes the corresponding public API declarations for the deleted loading helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -436,7 +436,6 @@ | |||
| size_t gather(size_t embeddings, size_t indices); | |||
| size_t mmap_embeddings(const std::string& filename); | |||
| size_t mmap_weights(const std::string& filename); | |||
There was a problem hiding this comment.
CactusGraph::load_weights was removed from a public header (graph.h). If this header is part of the external Cactus Graph API, this is a breaking change for downstream users. Consider keeping a thin deprecated wrapper that forwards to mmap_weights() (or clearly documenting the required migration for external callers).
| size_t mmap_weights(const std::string& filename); | |
| size_t mmap_weights(const std::string& filename); | |
| // Deprecated API retained for backward compatibility. Use mmap_weights() instead. | |
| #if defined(__cplusplus) && __cplusplus >= 201402L | |
| [[deprecated("load_weights() is deprecated; use mmap_weights() instead.")]] | |
| #endif | |
| inline size_t load_weights(const std::string& filename) { | |
| return mmap_weights(filename); | |
| } |
|
|
||
| void save_node(CactusGraph& graph, size_t node_id, const std::string& filename); | ||
| LoadedNode load_into_graph(CactusGraph& graph, const std::string& filename); | ||
|
|
There was a problem hiding this comment.
GraphFile::load_into_graph was removed from graph.h. Since this is also in a public header, it may break any external code that relied on non-mmap loading semantics (copying data into graph-owned buffers). If the removal is intentional, consider providing an alternative public API (or a deprecated compatibility shim) to avoid forcing external callers onto mmap-based lifetime behavior.
| struct LoadedNode { | ||
| size_t node_id; | ||
| std::vector<size_t> shape; | ||
| Precision precision; | ||
| size_t byte_size; | ||
| }; |
There was a problem hiding this comment.
GraphFile::LoadedNode now appears unused after removing the load helpers that returned it. Consider removing this struct as well (or repurposing it) to avoid leaving dead API surface in a public header.
CactusGraph::load_weights GraphFile::load_into_graph MappedFile::load_into_graph mmap_load Signed-off-by: Noah Cylich <[email protected]>
CactusGraph::load_weights GraphFile::load_into_graph MappedFile::load_into_graph mmap_load Signed-off-by: Noah Cylich <[email protected]>
Specifically removed the following:
CactusGraph::load_weights
GraphFile::load_into_graph MappedFile::load_into_graph mmap_load