Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow cugraph-nx to run networkx tests for nx versions 3.0, 3.1, and 3.2 #3808

Merged
merged 5 commits into from
Aug 22, 2023

Conversation

eriknw
Copy link
Contributor

@eriknw eriknw commented Aug 17, 2023

This is one step to support #3770

Note that NetworkX version 3.2 is currently under development.

CC @rlratzel

Note that NetworkX version 3.2 is currently under development.
@eriknw eriknw requested a review from a team as a code owner August 17, 2023 04:01
Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just had some ideas below.

python/cugraph-nx/cugraph_nx/interface.py Show resolved Hide resolved
@@ -51,17 +52,110 @@ def key(testpath):
return (testname, frozenset({classname, filename}))
return (testname, frozenset({filename}))

string_attribute = "unable to handle string attributes"
no_weights = "weighted implementation not currently supported"
no_multigraph = "multigraphs not currently supported"
Copy link
Contributor

@rlratzel rlratzel Aug 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is also a comment for all of on_start_tests(). No action for this PR beyond a potential FIXME if we like it.)

I'm assuming many backends will have potentially lengthy on_start_tests() implementations like this which use basically the same pattern and need frequent modification as the backends evolve to pass more tests.

I wonder if we could add a utility function to networkx which allows a backend developer to simply pass the incoming items and a YAML string and have them marked appropriately. The YAML could make it easy to define xfail messages for specific tests and nx versions, making these functions almost purely declarative and easier to read and maintain:

   from networkx.utils.backends import apply_xfails_from_yaml

   @staticmethod
   def on_start_tests(items):
      yaml = """
      nxver:
         3.2:
            test_betweenness_centrality.py:TestWeightedBetweennessCentrality.test_K5:
               weighted implementation not currently supported
      ...
      """
      apply_xfails_from_yaml(items, yaml)

The YAML could be inline like above, or in a separate file that's more discoverable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds nice; we could look into cleaning this up and making it easier for any backend to mark tests as xfail.

This seems pretty low priority for now--we'll see if I/we get frustrated by fiddling with this as we add more algorithms, and maybe we'll get motivated to make it better.

@rlratzel rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 21, 2023
@rlratzel
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit fa99e34 into rapidsai:branch-23.10 Aug 22, 2023
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants