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

[ENH] Re-org python test directory for consistency and ease-of-use #1481

Closed
Tracked by #3256
rlratzel opened this issue Mar 23, 2021 · 4 comments · Fixed by #3292
Closed
Tracked by #3256

[ENH] Re-org python test directory for consistency and ease-of-use #1481

rlratzel opened this issue Mar 23, 2021 · 4 comments · Fixed by #3292
Assignees
Labels
improvement Improvement / enhancement to an existing function
Milestone

Comments

@rlratzel
Copy link
Contributor

rlratzel commented Mar 23, 2021

Our CPP tests are organized under a nice directory hierarchy, and our python tests could benefit from a similar organization to make particular tests easier to find and run. It might also be nice if the test subdirs were organized similar to the source code.

The default method of running pytest to run all tests shouldn't change, since pytest auto-discovers tests under a directory hierarchy.

Also, by organizing using separate folders, users can run a subset of tests easily by specifying a particular folder, for example: pytest cugraph/tests/algos/community (in addition to the currently supported ways using markers, -k expressions, etc.) which encourages working with several smaller files instead of single larger ones.

Below is a proposed test dir hierarchy. Is this something worth doing, or do we prefer the relatively flat layout we have today, or maybe some combination?

python
|
|- cugraph
|  |
|  |- tests
|  |  |
|  |  |- algos (this includes both SG & MG)
|  |  |  |
|  |  |  |- centrality
|  |  |  |  |- test_betweenness_centrality
|  |  |  |  |- test_edge_betweenness_centrality
|  |  |  |  |- test_katz_centrality
|  |  |  |  |- test_mg_katz_centrality
|  |  |  |  |- test_mg_batch_betweenness_centrality.py
|  |  |  |  |- test_mg_batch_edge_betweenness_centrality
|  |  |  |
|  |  |  |- community
|  |  |  |  |- test_egonet
|  |  |  |  |- test_ecg
|  |  |  |  |- test_leiden
|  |  |  |  |- test_louvain
|  |  |  |  |- test_mg_louvain
|  |  |  |  |- test_modularity (rename to spectral clustering related)
|  |  |  |  |- test_subgraph_extraction (rename to test_subgraph?)
|  |  |  |  |- test_triangle_count
|  |  |  |  |- test_balanced_cut
|  |  |  |
|  |  |  |- components
|  |  |  |  |- test_connectivity
|  |  |  |
|  |  |  |- cores
|  |  |  |  |- test_core_number
|  |  |  |
|  |  |  |- layout
|  |  |  |  |- test_force_atlas2
|  |  |  |
|  |  |  |- link_analysis
|  |  |  |  |- test_hits
|  |  |  |  |- test_pagerank
|  |  |  |  |- test_mg_pagerank
|  |  |  |
|  |  |  |- link_prediction
|  |  |  |  |- test_jaccard
|  |  |  |  |- test_wjaccard
|  |  |  |  |- test_overlap
|  |  |  |  |- test_woverlap
|  |  |  |
|  |  |  |- traversal
|  |  |  |  |- test_bfs
|  |  |  |  |- test_mg_bfs
|  |  |  |  |- test_sssp
|  |  |  |  |- test_mg_sssp
|  |  |  |  |- test_paths (rename to test_shortest_path_length?)
|  |  |  |  |- test_traveling_salesperson
|  |  |  |  |- test_filter_unreachable
|  |  |  |
|  |  |  |- tree
|  |  |  |  |- test_maximum_spanning_tree
|  |  |  |  |- test_minimum_spanning_tree
|  |  |  |  
|  |  |  |- linear_assignment
|  |  |  |  |- test_hungarian
|  |  |
|  |  |- types (or structure?)
|  |  |  |- test_graph
|  |  |  |- test_hypergraph
|  |  |  |- test_multigraph
|  |  |  |- test_convert_matrix
|  |  |  |- test_symmetrize
|  |  |  |- test_mg_degree
|  |  |
|  |  |- internal (or infra?)
|  |  |  |- test_nx_convert
|  |  |  |- test_raft
|  |  |  |- test_renumber
|  |  |  |- test_utils
|  |  |  |- test_mg_utility
|  |  |  |- test_mg_renumber
|  |  |  |- test_mg_replication
|  |  |  |- test_mg_comms  (need to verify we need this since it seems identical to test_mg_pagerank)
|  |  |  |- [future] unit tests for graph infra classes
|  |  |  |- [future] unit tests for cython layer calls?
@rlratzel rlratzel added the ? - Needs Triage Need team to review and classify label Mar 23, 2021
@rlratzel rlratzel self-assigned this Mar 23, 2021
@BradReesWork BradReesWork added Refactor and removed ? - Needs Triage Need team to review and classify labels Mar 24, 2021
@BradReesWork
Copy link
Member

looks like all the MG tests are thrown into a single folder. Would it be worth doing something like
python
|
|- cugraph
| |
| |- tests
| | |-SG
....
| | |-MG
....

@rlratzel
Copy link
Contributor Author

rlratzel commented Mar 24, 2021

I put the MG and SG tests side-by-side with the other tests for each algo (so they're actually distributed across several folders, but not in a dedicated MG folder anymore). I thought it would be better that way to increase visibility - especially during refactoring work or when new tests are added - and since they're automatically excluded in SG environments anyway when run. I'm also hoping that in a later revision, we'll be able to have a single set of tests that can be used for both SG and MG environments, where MG algos are run simply by providing a dask dataframe and additional setup, instead of requiring an entirely different test, and this would push us in that direction.

@BradReesWork
Copy link
Member

BradReesWork commented Mar 24, 2021

If our CI environment is only SG, is there any easy way to skip those tests? Likewise, on the MNMG system, how are SG tests skipped?

I do like having everything together, just want to make sure that doesn't add complexity elsewhere

@rlratzel
Copy link
Contributor Author

rlratzel commented Mar 24, 2021

Right now our MG tests are automatically skipped if there's <2 GPUS detected using this decorator, so our current CI scripts and SG dev workflows wouldn't need to change. And today on MG systems, both SG and MG tests are run by default unless we manually exclude the SG tests (which we do). One advantage of separating them by folder is that you can specify just the MG ("dask") folder to exclude SG tests, but it's only a minor convenience IMO since you can do the same thing today using -k mg. Using the proposed folder layout above would require users to use -k mg if they want to skip SG tests, so that would be a minor change to our MG automation script which I'm happy to make for this.

I think in the long run, having SG and MG side-by-side will promote more MG testing, test maintenance, and get us to a single-source solution for both environments sooner.

@BradReesWork BradReesWork added the improvement Improvement / enhancement to an existing function label Feb 2, 2023
@BradReesWork BradReesWork added this to the 23.04 milestone Feb 2, 2023
@BradReesWork BradReesWork self-assigned this Feb 6, 2023
@rapidsai rapidsai deleted a comment from github-actions bot Feb 16, 2023
@rapidsai rapidsai deleted a comment from github-actions bot Feb 16, 2023
rapids-bot bot pushed a commit that referenced this issue Mar 10, 2023
Redoing the organization of the python tests.   

Merging both SG and MG together and using tag to specify runs
organizing tests by graph function 

**MG Test File Renaming**
- MG test files now end with "_mg" rather than starting with "test_mg".  This will make it easier to see that there are test pairs, SG and MG, since a listing will now group files together. 

**mg pytest marker**
- created a pytest marker called "mg" to identify MG test and allow CI/users to either include or skip
- `pytest -m mg` to run all mg tests
- `pytest -m "not mg"` to skip all MG tests

**sg marker**
test only SG code
pytest -m sg

Added markers for "sg" and "ci" for future use
Fixed a number of test issues


closes #1481

Authors:
  - Brad Rees (https://github.com/BradReesWork)

Approvers:
  - Ray Douglass (https://github.com/raydouglass)
  - Don Acosta (https://github.com/acostadon)
  - Rick Ratzel (https://github.com/rlratzel)
  - Joseph Nke (https://github.com/jnke2016)

URL: #3292
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants