Improve special cases in dispatch testing (paying off tech debt) #7982
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eriknw
added
type: Bug fix
type: Maintenance
Dispatching
eriknw
commented
Apr 17, 2025
dschult
reviewed
Apr 17, 2025
Contributor
Author
|
No changes to |
eriknw
commented
Apr 18, 2025
|
|
||
| @np_random_state(3) | ||
| @nx._dispatchable(returns_graph=True) | ||
| @nx._dispatchable(preserve_edge_attrs={"G": {"weight": 1}}, returns_graph=True) |
Contributor
Author
There was a problem hiding this comment.
Recall that whenever we see the pattern preserve_edge_attrs={"G": {"weight": 1}}, this means that is it using "weight" edge attributes implicitly, and the function may be a good candidate to add e.g. weight="weight" argument to.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes are intended to unblock #7902 (CC @rossbar) and to try to make dispatch tests more maintainable (see #7904).
This actually took a lot of experimentation, and I suspect there's even more debt we could pay off, but I think this is a nice increment. To summarize:
We are more methodological about when we create two different arguments to run with the backend and with networkx. As before, we run with networkx and compare results when the result is a graph. In this case, we check that the output graphs match exactly, and return the networkx graph in order to preserve iteration order (b/c some tests are sensitive). We are more careful about doing this when functions mutate input graphs--we now ensure that input graphs are different when calling the function with both the backend and with networkx. @rossbar, I think this is why the latest attempted "fix" for #7902 didn't work--an input graph was mutated, then used in a subsequent call.
This PR deletes a lot of weird, bespoke code that I suspect nobody understands and is likely to be difficult to maintain. This code was originally added so tests would pass. Now we have a different (and less bespoke) way for tests to pass.
This also adds a feature: we compare that input graphs are (very loosely) equal for functions that mutate input graphs. This makes the tests more strict and may affect backends.
Generally, making any changes to this code has a risk of affecting backends.
nx-cugraphhas a few test failures when run against this PR, but I think at least some are true failures.This code is still more complicated than I would prefer, but we're doing something pretty complex, and I believe enabling backends to run networkx tests is worth the trouble.