Add mutates_input= and returns_graph= to _dispatchable

[eriknw]

Knowing which functions mutate input graphs may be helpful when implementing (or testing) caching.

This changes a behavior: if a function is known to mutate an input graph, then this does not automatically convert an input graph to a backend graph. Conversion can still happen by using backend= keyword.

Some of the algorithms that now have mutates_input=True may be good candidates to add copy= arguments.

Adding mutates_input= to dispatch decorator is "best effort". It's possible (even likely) that some were missed, especially functions that add data to .graph. I think this is okay--we can fix them when found by backend implementers--but please share if you know of any other functions that mutate input graphs.

I noticed that negative_edge_cycle and held_karp_ascent temporarily mutate input graphs. I was surprised by this, and it means these are not thread-safe (and who knows if they may be permanently modified if there's an exception).

I also noticed that lukes_partitioning, is_kl_connected, and kl_connected_subgraph use deepcopy to copy the graph before mutating it. This is different than what is done elsewhere, and does not work if the input graphs have been frozen (side thought: why isn't there an unfreeze function?).

reverse function has copy= argument, but it does not mutate the graph. If copy=False, then a read-only view is returned.

Finally, I don't know what to do with minimum_cut. It can mutate residual if residual= was passed in via **kwargs. The dispatch machinery does not handle graphs passed in via **kwargs. We can punt on this until a backend wants to implement it.

CC @rlratzel

[eriknw]

Note that I moved this code block to further below so that iterators of input graphs are turned into lists. This allows the "fallback to networkx" functionality to work even for iterator of graphs inputs (otherwise, the iterators would be consumed by the first call).

[eriknw]

Note that this implicitly assumes e.g. copy=True is the default everywhere (which is true today).

[eriknw]

I added returns_graph=True to @nx._dispatchable like we discussed in the weekly dispatching meeting (although we didn't really discuss the argument name, so suggestions for a better name are welcome). I still need to change the logic to not convert-and-dispatch by default if a function returns a graph.

Even if we don't include graph generators/constructors, there were more functions that return graphs than I was expecting!

[eriknw]

Aha, our tests worked and found a new function that returns a graph (and needs returns_graph=True):
https://github.com/networkx/networkx/actions/runs/7700821863/job/20985472423?pr=7191#step:5:696

E           RuntimeError: `returns_graph` is incorrect for modular_product

This PR has been updated and is ready for review.

[rlratzel]

Thanks! This is a good topic for dispatching, especially as we work toward getting caching in for NX 3.3

This changes a behavior: if a function is known to mutate an input graph, then this does not automatically convert an input graph to a backend graph. Conversion can still happen by using backend= keyword.

Can you explain the reason for the behavior change? My understanding is that a function that mutates the input graph - as a side effect or otherwise - will not result in those updates being propagated to the NX Graph if dispatched, and therefore dispatching the call will result in different behavior (ie. a difference that breaks user code). What I'm not clear on is what the expectation is if a user forces the conversion/dispatch using backend=. Is this just use-at-your-own-risk, or are backends expected to properly mutate the NX Graph in that case? I'm assuming it's not the latter otherwise we'd then allow the automatic conversion/dispatch.

Either way, I think if a user sets NETWORKX_AUTOMATIC_BACKENDS=... then this new behavior will be surprising. Here's a couple of options:

• backends that can't mutate the NX graph should return False for can_run, which means being strict about treating Graph mutations as part of the contract for the function that backends have to honor. I suppose if that was decided, then we wouldn't need mutates_input= and we'd have to enforce the proper mutations with tests. (this feels like the most work, but the right decision IMO).
• somehow tell users we're making this decision for them: warning, logging, throw an exception by default which can be overridden by a config option, something else.

[rlratzel]

Chatted offline with @eriknw (Erik, please correct me as needed):

• The bigger goal is to not break user code, so the change here to not auto-dispatch will avoid that completely since backends are not currently assumed to mutate the input graph
• There's still an element of surprise for users using NETWORK_AUTOMATIC_BACKENDS=, but this is lower priority than preventing the breakage of user code
  • A future effort will be made to prevent this surprise too, and there's overlap with this and should_run
  • Introspection features ("tell me what you're going to do and why"), logging, and config are all topics/upcoming features related to solving this problem
• We want one of the overriding principles to be "if I tell you to do this then don't decide for me" and therefore backend= will still allow a user to force dispatch to a particular backend. This allows power users, backend developers, etc. to dispatch even if graph mutations aren't supported.