Add edge mutation and MutableSet interface for graph nodes

[Andy-Jost]

Summary

Implements step 2 of #1330 (graph updates): edge mutation on GraphDef. Graph node adjacencies (pred/succ) are now exposed as AdjacencySetProxy, a collections.abc.MutableSet proxy backed by cuGraphAddDependencies/cuGraphRemoveDependencies. Nodes can be removed via GraphNode.destroy() (wraps cuGraphDestroyNode), and property setters on pred/succ support bulk edge replacement.

Examples

# Inspect adjacencies
b in node.pred     # predecessors
b in node.succ     # successors

# Mutate edges
node.succ.add(b)           # add a single edge
node.succ |= {b, c}        # bulk add
node.succ.update({b, c})   # same thing
node.pred.discard(a)       # remove one predecessor
node.pred -= {a, b}        # bulk remove
node.succ = {x, y}         # replace all successors
node.pred.clear()          # remove all predecessors

# Remove a node and all its edges from a graph
node.destroy()

Changes

• AdjacencySetProxy: full MutableSet implementation with bulk-efficient clear(), __isub__(), __ior__(), update(), and remove_edges(). Includes _from_iterable override for binary ops and duplicate-add guard to avoid CUDA_ERROR_INVALID_VALUE.
• GraphNode.destroy(): removes a node and its edges from the graph definition via cuGraphDestroyNode, then zeroes the underlying handle to prevent stale memory access. Safe to call on already-destroyed nodes. Properties (type, pred, succ, handle) degrade gracefully to None or empty sets.
• GraphNode.is_valid: returns False after destroy() has been called
• GraphNode.pred/succ setters: clear existing edges and add new ones in bulk
• GraphDef.nodes()/edges(): return set instead of tuple for consistency
• Test helpers: compile_parallel_kernels() in graph_kernels.py, assert_mutable_set_interface() in collection_interface_testers.py

Test Coverage

• TestMutateYRig class: baseline, node destroy (single/multiple), edge insertion, join-node removal with rewiring
• test_destroyed_node: verifies handle invalidation, graceful property degradation, edge-add failure on destroyed nodes, and double-destroy safety
• test_adjacency_set_interface: exercises every MutableSet method against a reference set
• test_adjacency_set_pred_direction: verifies bidirectional edge visibility
• test_adjacency_set_property_setter: verifies bulk edge replacement via setters
• test_add_wrong_type, test_cross_graph_edge, test_self_edge: error-path coverage
• test_convert_linear_to_fan_in: rewires a sequential graph into parallel fan-in topology

Related Work

• Part of CUDA graph phase N - graph updates #1330 (step 2 of 4)
• Builds on the _graph_def/ subpackage refactor from graph-updates branch

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[Andy-Jost]

/ok to test

[rparolin]

Could we make the invalidate_graph_node_handle take a mutable reference? Rather than internally mutating through a const& handle?

[Andy-Jost]

I don't think it will improve anything. GraphNodeHandle is a std::shared_ptr, and this "invalidate" function relies on T& std::shared_ptr::operator*() const noexcept to get a non-const reference to the target object (note the function is marked const but the return type is a non-const T&; doc). Aconst qualifier on GraphNodeHandle only refers to whether the pointer can be reset or rebound to a new target through something like assignment/move, not whether the target itself is mutable. The "invalidate" function marks the target as having been invalidated by a call to cuGraphNodeDestroy but does not change the target location, so a const handle is correct.

Removing const from this function would not have the effect you might expect. Consider these three handles to two targets:

GraphNodeHandle h1 = ...;      // a graph node handle
const GraphNodeHandle h2 = h1; // a second handle to the same graph node
GraphNodeHandle h3 = ...;      // a handle to an unrelated graph node

Marking h2 with const protects the handle, not the GraphNode it points to. In this case, h2 = h3 would be disallowed. If we changed the declaration as you suggest, invalidate_graph_node_handle(h2) would be disallowed; however, invalidate_graph_node_handle(h1) would still be allowed and would invalidate the shared graph node.

In general, handle targets are always marked const because resource handles by design only permit attach, detach, and dereference operations. The GraphNodeHandle specifically and oddly needs invalidation because of the strange (I'd say broken) semantics of cuGraphNodeDestroy.

One way to support invalidation would be removing const from this declaration:

using GraphNodeHandle = std::shared_ptr<const CUgraphNode>

Then one could write *h2 = NULL to invalidate. That's not a good solution because it allows mutation everywhere.

The better approach (what's in this PR) is to keep the current definition, consistent with other handle types, while marking CUgraphNode as mutable in GraphNodeBox and providing an "invalidate" function to invalidate the handle when needed. This keeps the GraphNodeHandle similar to all other handles, while supporting this odd destroy behavior that is needed because of cuGraphNodeDestroy.

[Andy-Jost]

This function should probably be called invalidate_graph_node

[mdboom]

This may be premature optimization, but is it required to instantiate an entire list that is a copy of the elements on the C++ side just to test for in? We'd have to check that this works in Cython, but having query return an iterator comprehension rather than a list comprehension may be enough.

[mdboom]

Maybe this?

--- a/cuda_core/cuda/core/_graph/_graph_def/_adjacency_set_proxy.pyx
+++ b/cuda_core/cuda/core/_graph/_graph_def/_adjacency_set_proxy.pyx
@@ -68,7 +68,7 @@ class AdjacencySetProxy(MutableSet):
         """Remove all edges in a single driver call."""
         members = (<_AdjacencySetCore>self._core).query()
         if members:
-            (<_AdjacencySetCore>self._core).remove_edges(members)
+            (<_AdjacencySetCore>self._core).remove_edges(list(members))

     def __isub__(self, it):
         """Remove edges to all nodes in *it* in a single driver call."""
@@ -139,7 +139,7 @@ cdef class _AdjacencySetCore:
             c_from[0] = as_cu(other._h_node)
             c_to[0] = as_cu(self._h_node)

-    cdef list query(self):
+    cdef query(self):
         cdef cydriver.CUgraphNode c_node = as_cu(self._h_node)
         if c_node == NULL:
             return []
@@ -153,8 +153,8 @@ cdef class _AdjacencySetCore:
         with nogil:
             HANDLE_RETURN(self._query_fn(
                 c_node, nodes_vec.data(), &count))
-        return [GraphNode._create(self._h_graph, nodes_vec[i])
-                for i in range(count)]
+        return (GraphNode._create(self._h_graph, nodes_vec[i])
+                for i in range(count))

     cdef Py_ssize_t count(self):
         cdef cydriver.CUgraphNode c_node = as_cu(self._h_node)

[Andy-Jost]

I was thinking about this, too. Definitely a good idea to see whether query can return a generator. I was thinking of seeing whether I can implement contains() directly on AdjacencySetCore. We have to query the driver in batch, but we don't need to reconstruct the Python objects at all just to check containment.

[mdboom]

Could move this check next to the nodes.extend(other) above, since we already know it to be true if isinstance(other, GraphNode) above.

[mdboom]

discard is defined to silently do nothing if the thing being removed doesn't exist. And it definitely does this by experimentation, but it's puzzling to me /why/ it does, since the underlying call to cuGraphRemoveDependencies says:

"Specifying a non-existing dependency will return an error."

I'm probably just missing some step in the logic here that is (correctly) preventing the exception from bubbling up. But in the event that this is the API not doing what it says it would (and therefore might do so in the future), it's probably worth doing this twice to test that removing an already-moved element doesn't fail.

[Andy-Jost]

I had to put this containment check in discard for exactly this reason.

    def discard(self, value):
        if not isinstance(value, GraphNode):
            return
        if value not in self:  # <<<
            return

Also, the situation you mention is covered in assert_mutable_set_interface:

    # discard non-member is a no-op
    subject.discard(d)
    assert subject == ref

[mdboom]

Awesome. That's exactly the thing I missed.

[github-actions]

Doc Preview CI
Preview removed because the pull request was closed or merged.